#18013: Implement abstract tableau refactoring
-------------------------------------+-------------------------------------
Reporter: jpswanson | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.6
Component: combinatorics | Resolution:
Keywords: days64, sage- | Merged in:
combinat, tableaux, refactoring, | Reviewers:
days64.25, days68 | Work issues:
Authors: Josh Swanson | Commit:
Report Upstream: N/A | fff8dbfe4e579b801ee5f9a2faabec40a144242b
Branch: public/18013 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by tscrim):
* status: needs_review => needs_work
Comment:
Thank you for your work on this; this is something that needs to be done
(and Darij will be very happy once it is). However there are currently
many things that need to be addressed before this gets into Sage.
Do not remove the `__classcall_private__` for the parent; we want to
tightly couple that code with that class.
For the elements, they do need to know about their parents from a code
point of view, so it breaks some things that need to be done wrt the
categories. I also don't see the use of having a separate function which
we have to maintain as the creation of a tableau should be tightly coupled
with the tableau class.
In short, IMO the `__classcall_private__` (and `__classcall__`) idioms are
very useful for allowing easy public interface with the classes and
couples the (user) creation with the class. I strongly recommend
keeping/using them.
Next, from a quick look-over of the changes:
- Python is not Java, every class does not need to be in a separate file.
In fact, IMO the parent and element classes should be in the same file
because they are (loosely) coupled. I would also not put the global
options in a separate file as well, but with the abstract tableau(x).
- Not all methods have doctests (`_element_constructor_`'s in
`ribbon_shaped_tableau.py`).
- You need to add deprecations for the move as they are public functions.
- Remove the `list_method`, it is not used in `list(T)`, but instead
`T.list()`, which should be given by the category of `EnumeratedSets`. If
something isn't working without that, then you (we) need to do some
debugging.
- In the `__iter__`, you should not do `self(...)`, but instead
`self.element_class(self, ...)` as that way you don't invoke the coercion
framework (which should have some speed impact).
- In a similar vein, you should not iterate over another parent, but
instead refactor out the iterator into a function (generator/iterator)
which yields simple python lists. This will have a non-negligible effect
on speed because elements are "heavy" objects compared to python lists.
- The (typical) user will never see the documentation for
`_element_constructor_`; make each one have it's own and you can add a
`SEEALSO::` block to link it if you feel it is necessary.
- Your `_coerce_map_from_` is completely wrong (and poorly coded). Kill
with fire and holy water. It should not be a `@classmethod`. You're not
doing any arithmetic on tableaux, so I'm guessing you want coercions for
equality checks? Do a proper `_coerce_map_from_(self, P)` and check
`isinstance(P, Foo)`. For reference, then `Foo_with_category` is a
subclass of `Foo`.
- Demolish the factories as mentioned above.
- You're not initializing `AbstractTableau` correctly. You don't need an
`__init__` method because you want to inherit the one from `Element`. I
think you should remove the dependency on `six` in this class and make
more pythonic iterations. I'm also not completely convinced of how you're
not taking a `dict` as input, but that can come later after the other
issues are worked out.
- You should never have a `_set_parent` call, ever. This is a strong
indication you're doing something wrong.
There will likely be more changes that need to be done, but this will be a
good start.
--
Ticket URL: <http://trac.sagemath.org/ticket/18013#comment:19>
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/d/optout.
