#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 | 37ace348fb1afdb930b0657a80e40a58aea14be2
Branch: public/18013 | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Comment (by tscrim):
These "philosophical musings of OOP" that you are disregarding are there
to for good programming practices and to help ease the burden of code
maintenance. So they are real practical issues later on that will make our
lives easier when we need to change things yet again. So please don't
trivialize them. (I'm trying not to sound like a rant here, but I fear
I've failed.)
With that being said, in an ideal world, we would just allow the user
directly to the `__init__`, which could take a variety of different inputs
and initialize the class correctly as necessary. Python does not have
polymorphism, and this at least allows us to work around that for the user
entry point of `*Tableau` and still provide a good construction data from
the parent. The `__classcall_private__` is also there so you don't have a
''separate'' function with a ''different'' name (or at least in a separate
location which can lead to confusion as well). Even still, you are going
to either have to parse that same input in that separate function.
This is an implementation detail that, yes, does raise the barrier of
entry (which better documentation can alleviate), but the ease of code
maintenance because you know it's there with the class and behaves like a
constructor far outweighs the cost IMO. (BTW, if we ultimately decide to
have the creation functions, they should have `python_function_names` and
not `LookLikeClasses`.) The biggest problem is in a separate file you want
to create a, e.g., `SkewTableau` and want to check if something is an
instance of a `SkewTableau`, you have 2 different things for what really
should be 1 (although I would advocate creating the correct parent and
feeding the data to that parent, but I have seen this pattern a lot in
code in the wild). This is the "tightly coupling" I was talking about and
is the biggest way how your pattern breaks it.
The red flag that's popping up is more of a mathematical one in that we
are trying to provide an object for when the user does not care about the
set that object lies in. This idiom has made my life a lot easier in both
using the code and maintaining other combinatorics code that uses it, and
I strongly recommend keeping it.
In regards to the checking/duplication, what you should do is think about
how you want to code to flow through, what needs to be parsed, and at
which point. I'm not necessarily sure I like how you do your containment
checking because you are creating an element which you immediately
discard. This makes it a lot slower than it needs to be and you can
instead implement the necessary checks in various `__contains__` methods
(or common shortcuts in there, and have that call a `_contain_check`
method which you override in subclasses), and then have the
`_element_constructor_` just create an element and then call the
`__contains__`. I do understand why you did things this way, but there is
a price that is and will have to be paid.
Normalization should not be done in the parent; that is why you are doing
so much work. Let the element in the `__init__` do the normalization, put
it in a `@staticmethod` of the ''element'' and call that when needed, or
put it in one place that all code paths for element creation ''from the
user'' must go through. I think this will greatly simplify your code.
One other thing with the `_element_constructor_`, everywhere you basically
are checking if `check` is in the keywords. Please make `check` an
explicit named argument.
For splitting the parent/element classes up, a lot of those rings, look at
the file extensions. We want fast arithmetic and the like, which are done
on the elements, and so we use cython. However, cython has limitations and
don't (or at least didn't when those rings were created) behave well with
respect to categories and other pythonic things, so they were written in
python. (Again, most code in the rings folder is quite old.) Separate
files usually means separate concerns, but I feel their topic is close
enough together that they should be in the same file. Plus it helps keep
down the possibility of import loops (a major maintenance headache) and
startup imports/time.
The `self(x)` causes Sage to set the coercions for ``self`` from the
parent of ``x``, which has led to some very subtle bugs (e.g., #15309).
The speed bonus comes for free. Do not use unless you have a good reason
to, and in this case, I don't see a good one. As for the `__iter__`, this
I will insist must be changed before this gets into Sage, there are too
many subtleties and speed to be gained.
For the `_coerce_map_from_`, you will need to implement multiple such
methods in the appropriate parents or check more data if you want finer
control. This is a horrible hack and completely breaks the assumptions of
the coercion framework. It really smells more like you want simple
conversions with a good `__eq__` implementation, which will be faster at
the tradeoff of a little more code/work.
The answer to confusion in code is more/better documentation/comments.
For getting around `six`, you do things like `[d[i] for i in d]` or just
put `items` and deal with the slowdown until we (finally) convert to
python3. However I feel that your current design should implement a custom
`__getitem__` for the abstract tableaux and force all subclasses to use
that same internal data structure (that is part of the reason why they are
subclasses). If you want different data structures but to inherit methods,
python is nice with multiple inheritance. Create separate ABC's for each
internal type with the necessary methods for the actual concrete classes
you'd want. Otherwise you're going against well-established "philosophical
musings of OOP" and have to work harder than you need to for the things
you want.
Sorry this is more of a treatise than useful comments, but I think there
is a lot of work that will need to be done on this before this can be
merged into Sage. I understand that other things take precedence over
this, but I appreciate your work on this.
--
Ticket URL: <http://trac.sagemath.org/ticket/18013#comment:23>
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.
