#15143: Improvements to SetPartitions
------------------------------------+-----------------------------
Reporter: tscrim | Owner: sage-combinat
Type: enhancement | Status: needs_review
Priority: major | Milestone: sage-5.12
Component: combinatorics | Resolution:
Keywords: | Merged in:
Authors: Travis Scrimshaw | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
Dependencies: | Stopgaps:
------------------------------------+-----------------------------
Comment (by tscrim):
Thanks for looking at this patch.
Replying to [comment:2 darij]:
> Here is a started review. Before I continue, I'd like you to address
some questions:
>
> - Your {{{__mul__}}} method gives the same result as the existing
{{{inf}}} method, but seems to be faster at arriving there. Better to
reimplement the latter as an alias for the former? If you do so, the
docstrings should probably be merged.
I'll take a look at it (although both of these methods were prior
implementations).
> - I don't understand the docstring of
{{{ordered_set_partition_action}}}, nor do I understand the definition
given in [LM2011] (but I have not really tried). Maybe you can improve it
still? What are {{{A_{i_1}}}}, {{{A_{i_2}}}} etc.?
I'll expand on the docstring.
> - I have replaced the docstring and the implementation of
{{{is_atomic}}} to something that matches the definition of "atomic" in
the context of multiplicative generators of NCSym. I hope this is what you
really meant. First, the empty set partition is no longer atomic; second,
`[[1, 4], [2], [3]]` is now atomic despite max(2) < min(3) because there
is a max(1, 4) to spoil the game.
I was worried I oversimplified it. Thanks for fixing it.
Let's see if we can close a small can of worms. Well, since `1` usually
isn't considered as a generator of a unital algebra, it's okay that the
empty set partition is not atomic. I'm assuming that similarly `1` is not
considered a primitive of a Hopf algebra. However by the definition
directly, it would be considered atomic since there is no non-trivial
split, but I'll change it to state explicitly we don't consider it atomic.
This is similar to how we don't consider `1` as being prime.
> - I have removed the part about set compositions in the docstring of
{{{__mul__}}} because the method only appears on the SetPartition class.
>
> - I see you have changed the parent of all set partitions to not include
the base set. I am not saying this is wrong; just letting you know that I
cannot fully test the consequences of this change and so my review will be
somewhat incomplete even when it is finished.
I made sure to put in some tests for things like equality and for the
`is_less_than` and `is_strict_refinement` methods when comparing objects
with different parents. Doing it this way will net a (very) small speedup
in creation since we don't have to find the right parent by taking the
union of sets (I didn't measure). It also made me feel more secure in my
implementation of `NCSym`.
I'll post the new version of the patch tomorrow.
--
Ticket URL: <http://trac.sagemath.org/ticket/15143#comment:3>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/groups/opt_out.
