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:

Status badges

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 dkrenn

  • Branch set to u/dkrenn/extend-pushout-cartesian-growth-group

comment:2 Changed 2 years ago by dkrenn

  • Cc behackl cheuberg added
  • Commit set to 41a6dc0c8327a32aa91a74b89f6cb51a23a867d6
  • Status changed from new to needs_review

New commits:

a1e8f9aTrac #26681: merge sorted routine
41a6dc0Trac #26681: use new routine to extend pushouts of cartesian products of growth groups

comment:3 Changed 2 years ago by dkrenn

  • Dependencies set to #26682

comment:4 Changed 2 years ago by git

  • Commit changed from 41a6dc0c8327a32aa91a74b89f6cb51a23a867d6 to ae9f897d9a2fb16eb38f3dc3319bef08f795323b

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

436faeeTrac #26682: fixup logarithmic factor
6d1d858Merge branch 't/26682/zeta-from-group-8.4' into t/26682/zeta-from-group
c994b45Merge tag '8.7' into u/dkrenn/asy-roots-of-unity-etc
6b50c47Trac #26587: rename variable in doctest
01aef0fMerge tag '8.7' into u/dkrenn/roots-of-unity-group
48523adextend conversion repr<-->parent (cherry-pick)
9aab729remove import
a5fefd2Merge branch 'u/dkrenn/roots-of-unity-group' into HEAD
3cb9cc4Merge branch 'u/dkrenn/asy-roots-of-unity-etc' into u/dkrenn/zeta-from-group
ae9f897Merge branch 'u/dkrenn/zeta-from-group' into u/dkrenn/extend-pushout-cartesian-growth-group

comment:5 Changed 2 years ago by git

  • Commit changed from ae9f897d9a2fb16eb38f3dc3319bef08f795323b to 49e5d9c39dba00415ef696d643440df889c22e8b

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

7c1a021fix failing doctest (QQ^y --> (QQ_+)^y)
8a6b4caTrac #26587: use isidentifier
7620a4fTrac #26588: kwds passed on to element during construction
3d72ec3Trac #26588: return type of __abs__
31668d6Trac #26588: move exactly_one_is_true to sage.misc.misc
9d7173dTrac #26588: add doctest for normalize=False
1c3e52fMerge branch 't/26588/asy/roots-of-unity-groups' into t/26587/asy/roots-of-unity-etc
22e2957Trac #26587: handle multiple spaces in growth group string
95db9b7Merge branch 't/26587/asy/roots-of-unity-etc' into t/26682/zeta-from-group
49e5d9cMerge branch 't/26682/zeta-from-group' into t/26681/extend-pushout-cartesian-growth-group

comment:6 follow-up: Changed 2 years ago by behackl

  • 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 git

  • Commit changed from 49e5d9c39dba00415ef696d643440df889c22e8b to 785aa20baff8fe85e01dce534437bab09304b1ed

Branch pushed to git repo; I updated commit sha1. New commits:

baef62cTrac #26681: extend doc of merge_sorted
785aa20Trac #26681: rename to bidrectional_merge_*

comment:8 in reply to: ↑ 6 Changed 2 years ago by dkrenn

  • Status changed from needs_work to needs_review

Replying to behackl:

The merge_sorted method needs a better, more explicit documentation.

Extended documentation in

baef62cTrac #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:

785aa20Trac #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

baef62cTrac #26681: extend doc of merge_sorted

comment:9 Changed 2 years ago by behackl

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 dkrenn

  • Status changed from needs_review to positive_review

comment:11 Changed 2 years ago by vbraun

  • Branch changed from u/dkrenn/extend-pushout-cartesian-growth-group to 785aa20baff8fe85e01dce534437bab09304b1ed
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.