Opened 2 years ago
Closed 2 years ago
#26681 closed defect (fixed)
extend pushout constructions of growth groups
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.8 |
Component: | asymptotic expansions | Keywords: | |
Cc: | behackl, cheuberg | Merged in: | |
Authors: | Daniel Krenn | Reviewers: | Benjamin Hackl |
Report Upstream: | N/A | Work issues: | |
Branch: | 785aa20 (Commits, GitHub, GitLab) | Commit: | 785aa20baff8fe85e01dce534437bab09304b1ed |
Dependencies: | #26682 | Stopgaps: |
Description
Make
sage: cm.common_parent(GrowthGroup('n^ZZ * log(n)^ZZ * U^n'), ....: GrowthGroup('n^QQ * U^n')) Growth Group n^QQ * log(n)^ZZ * U^n
work. (The existing implementation only works in special cases.)
Change History (11)
comment:1 Changed 2 years ago by
- Branch set to u/dkrenn/extend-pushout-cartesian-growth-group
comment:2 Changed 2 years ago by
- Cc behackl cheuberg added
- Commit set to 41a6dc0c8327a32aa91a74b89f6cb51a23a867d6
- Status changed from new to needs_review
comment:3 Changed 2 years ago by
- Dependencies set to #26682
comment:4 Changed 2 years ago by
- Commit changed from 41a6dc0c8327a32aa91a74b89f6cb51a23a867d6 to ae9f897d9a2fb16eb38f3dc3319bef08f795323b
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
436faee | Trac #26682: fixup logarithmic factor
|
6d1d858 | Merge branch 't/26682/zeta-from-group-8.4' into t/26682/zeta-from-group
|
c994b45 | Merge tag '8.7' into u/dkrenn/asy-roots-of-unity-etc
|
6b50c47 | Trac #26587: rename variable in doctest
|
01aef0f | Merge tag '8.7' into u/dkrenn/roots-of-unity-group
|
48523ad | extend conversion repr<-->parent (cherry-pick)
|
9aab729 | remove import
|
a5fefd2 | Merge branch 'u/dkrenn/roots-of-unity-group' into HEAD
|
3cb9cc4 | Merge branch 'u/dkrenn/asy-roots-of-unity-etc' into u/dkrenn/zeta-from-group
|
ae9f897 | Merge branch 'u/dkrenn/zeta-from-group' into u/dkrenn/extend-pushout-cartesian-growth-group
|
comment:5 Changed 2 years ago by
- Commit changed from ae9f897d9a2fb16eb38f3dc3319bef08f795323b to 49e5d9c39dba00415ef696d643440df889c22e8b
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
7c1a021 | fix failing doctest (QQ^y --> (QQ_+)^y)
|
8a6b4ca | Trac #26587: use isidentifier
|
7620a4f | Trac #26588: kwds passed on to element during construction
|
3d72ec3 | Trac #26588: return type of __abs__
|
31668d6 | Trac #26588: move exactly_one_is_true to sage.misc.misc
|
9d7173d | Trac #26588: add doctest for normalize=False
|
1c3e52f | Merge branch 't/26588/asy/roots-of-unity-groups' into t/26587/asy/roots-of-unity-etc
|
22e2957 | Trac #26587: handle multiple spaces in growth group string
|
95db9b7 | Merge branch 't/26587/asy/roots-of-unity-etc' into t/26682/zeta-from-group
|
49e5d9c | Merge branch 't/26682/zeta-from-group' into t/26681/extend-pushout-cartesian-growth-group
|
comment:6 follow-up: ↓ 8 Changed 2 years ago by
- Milestone changed from sage-8.5 to sage-8.8
- Reviewers set to Benjamin Hackl
- Status changed from needs_review to needs_work
The merge_sorted
method needs a better, more explicit documentation. The name of the method leads me to believe that something like a MergeSort? step is happining. In some sense this is the case, however, it happens in both directions and elements with the same key are kept in the respective list. This should be reflected in the docstring.
Also, please document that the lists are required to have some overlap (w.r.t. the comparison keys) and otherwise raises an error.
The code itself is fine and also the doctests are good. (And of course, the changes in growth_group_cartesian.py
solve the problem from the description.)
comment:7 Changed 2 years ago by
- Commit changed from 49e5d9c39dba00415ef696d643440df889c22e8b to 785aa20baff8fe85e01dce534437bab09304b1ed
comment:8 in reply to: ↑ 6 Changed 2 years ago by
- Status changed from needs_work to needs_review
Replying to behackl:
The
merge_sorted
method needs a better, more explicit documentation.
Extended documentation in
baef62c | Trac #26681: extend doc of merge_sorted
|
The name of the method leads me to believe that something like a MergeSort? step is happining. In some sense this is the case, however, it happens in both directions and elements with the same key are kept in the respective list.
Renamed in:
785aa20 | Trac #26681: rename to bidrectional_merge_*
|
Also, please document that the lists are required to have some overlap (w.r.t. the comparison keys) and otherwise raises an error.
Also done in
baef62c | Trac #26681: extend doc of merge_sorted
|
comment:9 Changed 2 years ago by
Thank you for the changes, I am fine with the method now.
Conditional positive_review
; waiting for the patchbot.
comment:10 Changed 2 years ago by
- Status changed from needs_review to positive_review
comment:11 Changed 2 years ago by
- Branch changed from u/dkrenn/extend-pushout-cartesian-growth-group to 785aa20baff8fe85e01dce534437bab09304b1ed
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Trac #26681: merge sorted routine
Trac #26681: use new routine to extend pushouts of cartesian products of growth groups