#18109: Restructure IntegerListLex code
-------------------------------------+-------------------------------------
Reporter: vdelecroix | Owner:
Type: defect | Status: needs_review
Priority: major | Milestone: sage-6.6
Component: combinatorics | Resolution:
Keywords: | Merged in:
Authors: Jeroen Demeyer | Reviewers:
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/jdemeyer/ticket/18109 | 9582a7a5248f609a2d2de02497b4b1d36d9dd96e
Dependencies: #18181, #18184 | Stopgaps:
-------------------------------------+-------------------------------------
Comment (by nthiery):
Hi Jeroen,
Sorry, I got sidetracked reviewing other tickets. Thanks Anne for the
reminder, and for checking the code and the documentation!
I have just been through the code myself (not checking the fine
details though).
Altogether, I believe it's correct. Cythonizing the envelope function
and making the iterator into a Cython class that depends on little of
the Sage infrastructure was in our long term plans, so that's great (I
would have done it after the algorithm refactorisation, but since it's
done, let's move forward). Generalizing a base IntegerLists class is
useful as well.
On the other hand, is it absolutely necessary to have this separation
between frontend and backend classes? It introduces some complexity
and in fact some duplication (e.g. in the tests). In particular, I am
worried that it might introduce complexity in the construction of new
special case classes based on `IntegerListsLex`, when the primary goal
is to make this as trivial as possible. I'd need to actually implement
such new classes to qualify this worry.
If it's just a question of avoiding calls to the element constructor,
could not this be done just locally in `__iter__` (returning directly
an `IntegerListsLexIter` object, or mapping the constructor on it)?
If it's a question of making the iterator code independent of
`Parent`, well, it already is: it only uses the `IntegerListsLex`
object as a data holder for the constraints, right? As long as we
don't have a specific use case (backed up by benchmarks if speed is
the motivation), there may not be much point in maintaining additional
code for this.
At the very least, there should be some entry point (typically in the
module description) listing all the classes, and clarifying their
respective roles. Maybe slightly more systematic class names could
help; say: `IntegerLists`, `IntegerListsLex`, `IntegerListsBackend`,
`IntegerListsLexBackend`.
Some small suggestions:
- some of the cdef attributes could be declared as
int's. E.g. `Envelope.min_length`.
- maybe we could use the occasion to rename the attribute
`IntegerListsLexIter.parent` especially if it's not a parent
anymore.
- Another small complication is having to handle pickling by hand each
time. At least, would there be a way to implement `XXX.__reduce__`
to return `(XXX, *)` rather than having to implement an
`unpickle_XXX` function each time?
Cheers,
Nicolas
--
Ticket URL: <http://trac.sagemath.org/ticket/18109#comment:44>
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.
