#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: Benjamin Hackl
Authors: Daniel Krenn | Work issues:
Report Upstream: N/A | Commit:
Branch: u/behackl/asy | c1f8877b7dcea0e1c677d350d7f5d41be02169c8
/mutable-poset | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by dkrenn):
* branch: u/dkrenn/asy/mutable-poset => u/behackl/asy/mutable-poset
Comment:
Replying to [comment:25 behackl]:
> - `MutablePoset`, `MutablePosetShell`: derived from `object` vs.
`SageObject`?
`object`.
> - `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.
Keyword deleted.
> - doctests for `MutablePosetShell.le`: shouldn't doctests like `elem1 <=
elem2` be marked as indirect?
Yes, I've marked them.
> - `MutablePosetShell._copy_all_linked_`: 1st sentence should be a short
description. Likewise for `MutablePosetShell._iter_depth_first_visit_` and
`MutablePosetShell._iter_topological_visit_`.
Rewritten.
> - 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.
`sorted_set_by_tuple`: Deleted and replaced.
`is_MutablePoset`: seems to be a standard convention to use it in Sage;
see a lot of other modules.
> - `MutablePoset.shells`: I do not understand the usecase of the
`reverse` keyword. Can it be removed?
I agree; deleted.
> - `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.
I've added a comment.
> - `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.
See Python's set (the same behavior there).
> - 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)`.
Changed all to self/other.
> - 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.
I've added a note in all these functions.
> - `MutablePoset.mapped`: why is this function needed? Purely for
semantical reasons? I'd move the doctests to `MutablePoset.copy`.
It is a shortcut for using copy with the mapping argument (since this is a
non-standard argument for a copy method, including a separate method with
an approriate name seems to be a good choice)
> - `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.
Yes it is standard; see (all) deepcopy routines.
> Also, I've made a few reviewer commits concerning the documentation; the
branch is attached.
Cross-reviewed. Thanks.
--
Ticket URL: <http://trac.sagemath.org/ticket/17693#comment:29>
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.
