#17693: mutable poset: a data structure for asymptotic expressions
-------------------------------------+-------------------------------------
Reporter: dkrenn | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.5
Component: asymptotic | Resolution:
expansions | Merged in:
Keywords: asymptotics | Reviewers:
Authors: Daniel Krenn | Work issues:
Report Upstream: N/A | Commit:
Branch: u/behackl/asy | c1f8877b7dcea0e1c677d350d7f5d41be02169c8
/mutable-poset | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by behackl):
* status: needs_review => needs_work
* commit: ceb0b37bc7640f4300962a89eabefe5abc7263d1 =>
c1f8877b7dcea0e1c677d350d7f5d41be02169c8
* branch: u/dkrenn/asy/mutable-poset => u/behackl/asy/mutable-poset
Comment:
Hello!
I reviewed this ticket, and here are my comments:
- `MutablePoset`, `MutablePosetShell`: derived from `object` vs.
`SageObject`?
- `is_null`, `is_oo`: is there some usecase for the `reverse` keyword? For
`predecessor` and `successor`: this is clear, as it makes the code easier.
However, I think that a `reverse`-keyword for `is_null` and `is_oo` is
rather irritating.
- doctests for `MutablePosetShell.le`: shouldn't doctests like `elem1 <=
elem2` be marked as indirect?
- `MutablePosetShell._copy_all_linked_`: 1st sentence should be a short
description. Likewise for `MutablePosetShell._iter_depth_first_visit_` and
`MutablePosetShell._iter_topological_visit_`.
- sorted_set_by_tuple, is_MutablePoset: are these functions really
necessary? Both are only used once within the whole file. The occurrence
of `sorted_set_by_tuple` can be easily replaced by
{{{
[...].join(repr(e) for e in sortedshells if e in shell.successors(rev))
}}}
and the occurrence of `is_MutablePoset` can be replaced by a simple
`isinstance`-call.
- `MutablePoset.shells`: I do not understand the usecase of the `reverse`
keyword. Can it be removed?
- `MutablePoset.add`: I think that a comment (in the code) that roughly
explains what the passage beginning with `for reverse in (False, True):`
does would be nice. It took me quite some time to understand and verify
the functionality of this block.
- `MutablePoset.discard`: why isn't this simply an alias of
`MutablePoset.remove`? As `remove` has no return value, `discard` cannot
have one either, if that was the intention.
- Is there a particular reason for naming the parameters for all the set
operations like `MutablePoset.union`, `MutablePoset.difference` etc.
`left` and `right`, instead of `self` and `other`? IMHO, this disguises
that the function can be called as `P.union(Q)`.
- Regarding all set operations: currently, it is **not** clear from the
documentation that all set operations are performed on the set of keys.
This causes something like
{{{
sage: PR.<x> = ZZ[]
sage: P = MutablePoset(key=lambda k: len(k.list()))
sage: P.add(42*x)
sage: P.add(21*x^2)
sage: Q = MutablePoset(key=lambda k: len(k.list()))
sage: Q.add(1111*x)
sage: P, Q
(poset(42*x, 21*x^2), poset(1111*x))
sage: Q.is_subset(P)
True
sage: Q.union(P)
poset(1111*x, 21*x^2)
}}}
and so on. In principle, I don't see this as a problem---however, I'd
state this explicitly in each of these set operation functions.
- `MutablePoset.mapped`: why is this function needed? Purely for
semantical reasons? I'd move the doctests to `MutablePoset.copy`.
- `MutablePosetShell._copy_all_linked_`: is the memoization using the `id`
of the object a standard procedure? I thought about it, but I'm not
entirely sure that this isn't a potential error source.
Side note: the `MutablePoset` is the central data structure in our
!AsymptoticExpansions framework (cf. #17601). In this context, the
functionality has been tested extensively--and some peculiarities (like
the `map`-function etc.) are motivated by this application.
Also, I've made a few reviewer commits concerning the documentation; the
branch is attached.
As soon as the points I've raised in the comments above are discussed,
this would be a `positive_review` from my side.
Benjamin
----
Last 10 new commits:
||[http://git.sagemath.org/sage.git/commit/?id=e64cbd80ca50a19544af525b6da681ec7dfe607d
e64cbd8]||{{{mapping argument for .copy()}}}||
||[http://git.sagemath.org/sage.git/commit/?id=c28749c93a74337e1de987419d5d6e8760c55317
c28749c]||{{{bring map and mapped back to work and use new copy-
construction}}}||
||[http://git.sagemath.org/sage.git/commit/?id=ceb0b37bc7640f4300962a89eabefe5abc7263d1
ceb0b37]||{{{fix bug in merge}}}||
||[http://git.sagemath.org/sage.git/commit/?id=525bf3c334cf2f62a63999cbd5dc687c709a5dea
525bf3c]||{{{Merge branch 'u/dkrenn/asy/mutable-poset' of
git://trac.sagemath.org/sage into 6.9.beta5}}}||
||[http://git.sagemath.org/sage.git/commit/?id=bf1928f7a9ef5eac28a8ed419ef13cb97cfbe806
bf1928f]||{{{remove unused import}}}||
||[http://git.sagemath.org/sage.git/commit/?id=98a7939651645c24263e36c8fca81dc1ba6d9a96
98a7939]||{{{print something --> print(something)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=2a7ab889e212062323f79c318596d5413ae7e1ea
2a7ab88]||{{{several improvements to documentation (language,
structure)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=a4c43b71c81cc75b152a99d80754c1cb47104613
a4c43b7]||{{{fixed two examples-blocks}}}||
||[http://git.sagemath.org/sage.git/commit/?id=a867d3a36f5a068b6e7e3d00f95a5c4331960a6b
a867d3a]||{{{indentation (PEP 8)}}}||
||[http://git.sagemath.org/sage.git/commit/?id=c1f8877b7dcea0e1c677d350d7f5d41be02169c8
c1f8877]||{{{improved errors and added two doctests}}}||
--
Ticket URL: <http://trac.sagemath.org/ticket/17693#comment:25>
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.
