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:  sage8.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/extendpushoutcartesiangrowthgroup
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/zetafromgroup8.4' into t/26682/zetafromgroup

c994b45  Merge tag '8.7' into u/dkrenn/asyrootsofunityetc

6b50c47  Trac #26587: rename variable in doctest

01aef0f  Merge tag '8.7' into u/dkrenn/rootsofunitygroup

48523ad  extend conversion repr<>parent (cherrypick)

9aab729  remove import

a5fefd2  Merge branch 'u/dkrenn/rootsofunitygroup' into HEAD

3cb9cc4  Merge branch 'u/dkrenn/asyrootsofunityetc' into u/dkrenn/zetafromgroup

ae9f897  Merge branch 'u/dkrenn/zetafromgroup' into u/dkrenn/extendpushoutcartesiangrowthgroup

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/rootsofunitygroups' into t/26587/asy/rootsofunityetc

22e2957  Trac #26587: handle multiple spaces in growth group string

95db9b7  Merge branch 't/26587/asy/rootsofunityetc' into t/26682/zetafromgroup

49e5d9c  Merge branch 't/26682/zetafromgroup' into t/26681/extendpushoutcartesiangrowthgroup

comment:6 followup: ↓ 8 Changed 2 years ago by
 Milestone changed from sage8.5 to sage8.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/extendpushoutcartesiangrowthgroup 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