Opened 10 months ago
Closed 5 months ago
#32278 closed enhancement (fixed)
Add support for Bterms in Asymptotic Ring
Reported by:  ghthhagelmayer  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  asymptotic expansions  Keywords:  gsoc2021, asymptotics 
Cc:  cheuberg, behackl, dkrenn  Merged in:  
Authors:  Thomas Hagelmayer  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  2474870 (Commits, GitHub, GitLab)  Commit:  24748702f01276cdc678c969ff74482c3c8d347b 
Dependencies:  #31933, #32229  Stopgaps: 
Description (last modified by )
Add support for Bterms in Asymptotic Ring.
Change History (23)
comment:1 Changed 10 months ago by
 Component changed from PLEASE CHANGE to asymptotic expansions
 Keywords gsoc21 asymptotics added
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 10 months ago by
 Branch set to u/ghthhagelmayer/add_support_for_b_terms_in_asymptotic_ring
comment:3 followup: ↓ 4 Changed 10 months ago by
 Cc cheuberg behackl dkrenn added
 Commit set to 5c5079df6021937e33f0a31f8768169e8128c21d
 Dependencies set to #31933
 Description modified (diff)
 Status changed from new to needs_info
comment:4 in reply to: ↑ 3 Changed 10 months ago by
Replying to ghthhagelmayer:
I have implemented the initial functions for the usage of
Bterms
inAsymptoticRing
.I have three doctest errors:
File "src/sage/rings/asymptotic/asymptotic_ring.py", line 4691, in sage.rings.asymptotic.asymptotic_ring.AsymptoticRing.B Failed example: A.B(2*x^3, {x: 5}) Exception raised: Traceback (most recent call last): ... TypeError: _create_element_() missing 1 required positional argument: 'valid_from'The other two are the same from
src/sage/rings/asymptotic/asymptotic_ring.py", line 3379
andsrc/sage/rings/asymptotic/asymptotic_ring.py", line 3381
.
My guess, it seems like they could go away by #32229. (But anyways, it would need to be A.B(2*x^3, valid_from={x: 5})
.
comment:5 Changed 10 months ago by
 Commit changed from 5c5079df6021937e33f0a31f8768169e8128c21d to 1e201eaf6b11e290b2d331cb8ba391ee1f65e5ce
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
d076a0d  Trac #31933. Item 83: remove unused import

343ddd8  Trac #31933. Item 84: correct spelling

ff4747b  Trac #31933. Item 86: make fstring an frstring

f62e7ed  Trac #31933. Item 87: use the same description for MonomialGrowthElement._find_minimum_ as in GenericGrowthElement

832c80f  Trac #31933. Item 88: remove 'the' in the acknowledgements to have consistency

2e46c20  Trac #31933. Item 85: BTerm.__init__: Replace >= by \ge in all TeX formatted code

309d1e1  Merge branch 't/31933/initial_support_for_bterms' into t/32229/bterms_conversion

9e5b3fb  Trac #32229: fix doctests

766067f  Merge branch 't/32229/bterms_conversion' into t/32278/add_support_for_b_terms_in_asymptotic_ring

1e201ea  Trac #32278: fix doctests

comment:6 Changed 10 months ago by
 Dependencies changed from #31933 to #31933, #32229
 Milestone changed from sage9.4 to sage9.5
 Status changed from needs_info to needs_review
I merged #32229 into this ticket. Afterwards I could fix the doctests which were failing.
Last 10 new commits:
d076a0d  Trac #31933. Item 83: remove unused import

343ddd8  Trac #31933. Item 84: correct spelling

ff4747b  Trac #31933. Item 86: make fstring an frstring

f62e7ed  Trac #31933. Item 87: use the same description for MonomialGrowthElement._find_minimum_ as in GenericGrowthElement

832c80f  Trac #31933. Item 88: remove 'the' in the acknowledgements to have consistency

2e46c20  Trac #31933. Item 85: BTerm.__init__: Replace >= by \ge in all TeX formatted code

309d1e1  Merge branch 't/31933/initial_support_for_bterms' into t/32229/bterms_conversion

9e5b3fb  Trac #32229: fix doctests

766067f  Merge branch 't/32229/bterms_conversion' into t/32278/add_support_for_b_terms_in_asymptotic_ring

1e201ea  Trac #32278: fix doctests

comment:7 Changed 10 months ago by
 Commit changed from 1e201eaf6b11e290b2d331cb8ba391ee1f65e5ce to 6d9ea893fb05b0c56aa3a9308f0dfbc8f494d4fa
Branch pushed to git repo; I updated commit sha1. New commits:
4798c10  Trac #32278. moved valid_from logic from asymptotic_ring to term_monoid. corrected a doctest.

ca748e2  Trac #32278: Fix doctests in misc.py

6d9ea89  Trac #32278. moved logic from asymptoic_ring.py to term_monoid.py. fixed non working tests.

comment:8 Changed 10 months ago by
I moved the logic from asymptotic_ring
to term_monoid
and fixed the doctests.
comment:9 Changed 10 months ago by
 Keywords gsoc2021 added; gsoc21 removed
comment:10 followups: ↓ 12 ↓ 20 Changed 9 months ago by
 Status changed from needs_review to needs_work
This looks pretty functional already. Here are some remarks:
AsymptoticExpansion.B
: The tests in the docstring useAsymptoticRing.B
(which is fine in principle, but the corresponding lines should be marked with# indirect doctest
then)  either do this, or alternatively rewrite the tests to useAsymptoticExpansion.B
.
AsymptoticExpansion.B
,AsymptoticRing.B
,BTerm
: The docstring, in particular: the description ofvalid_from
, should mention the "short syntax" where passing a number instead of a dictionary uses the number as the lower bound for all variables occurring in the term.
AsymptoticExpansion.B
 Is there a particular reason thatvalid_from
is just a positional argument, and not a keyword argument with default value 0 (like forAsymptoticRing.B
?)
NotImplementedBZeroError
: Maybe add a straightforward test likesage: AR.<n> = AsymptoticRing('n^QQ', QQ) sage: AR(0).B(42) Traceback (most recent call last): ... NotImplementedBZero: got B(0) The error term B(0) means 0 for sufficiently large n.
comment:11 Changed 9 months ago by
 Commit changed from 6d9ea893fb05b0c56aa3a9308f0dfbc8f494d4fa to 28c3984985d2ea7c1e1af1641d92eabe16f8d431
Branch pushed to git repo; I updated commit sha1. New commits:
586ef6f  Trac #32278. Item 4: add test for NotImplementedBZero Error

351d659  Trac #32278. Item 1: add comment for indirect doctest

28c3984  Trac #32278. Items 2 and 3: added information of the short syntax to the docstring. AsymptoticExpansion.B make valid_from a keyword argument with default value 0

comment:12 in reply to: ↑ 10 Changed 9 months ago by
 Status changed from needs_work to needs_review
Replying to behackl:
Thank you for the comments.
AsymptoticExpansion.B
 Is there a particular reason thatvalid_from
is just a positional argument, and not a keyword argument with default value 0 (like forAsymptoticRing.B
?)
I thought it is not important to make the valid_from
a keyword argument in AsymptoticExpansion.B
, since it is already a keyword argument in AsymptoticRing.B
. I changed it now, because I think it is more correct to have it this way.
comment:13 followup: ↓ 15 Changed 9 months ago by
 Status changed from needs_review to needs_work
since it is already a keyword argument in
AsymptoticRing.B
I see. I think it makes sense to make sure the type of arguments are consistent across the methods.
sage: (2*x).B({x: 20}) # indirect doctest
 this is actually not indirect, there you do useAsymptoticExpansion.B
.
 "If you pass only a number as
valid_from
": I suggest to rephrase this as "If a number is passed tovalid_from
, ...".
 (2., continued): The docstring of
BTerm
should also be changed accordingly.
comment:14 Changed 9 months ago by
 Commit changed from 28c3984985d2ea7c1e1af1641d92eabe16f8d431 to 52b734d2d2c16435d9ce7fc1435ad830ad474ea9
comment:15 in reply to: ↑ 13 Changed 9 months ago by
 Status changed from needs_work to needs_review
sage: (2*x).B({x: 20}) # indirect doctest
 this is actually not indirect, there you do useAsymptoticExpansion.B
.
 "If you pass only a number as
valid_from
": I suggest to rephrase this as "If a number is passed tovalid_from
, ...".
 (2., continued): The docstring of
BTerm
should also be changed accordingly.
I have addressed these items.
comment:16 Changed 9 months ago by
 Branch changed from u/ghthhagelmayer/add_support_for_b_terms_in_asymptotic_ring to u/behackl/add_support_for_b_terms_in_asymptotic_ring
comment:17 Changed 9 months ago by
 Commit changed from 52b734d2d2c16435d9ce7fc1435ad830ad474ea9 to a57a2751c7585da416bcf86d611a94b506023b99
 Reviewers set to Benjamin Hackl
 Status changed from needs_review to positive_review
I've added one small commit to resolve a wording problem in the docstring, please crosscheck.
Otherwise, this looks good to me now, thank you.
New commits:
a57a275  Trac #32278: fix singular/plural problem in docstrings

comment:18 Changed 9 months ago by
 Commit changed from a57a2751c7585da416bcf86d611a94b506023b99 to 24748702f01276cdc678c969ff74482c3c8d347b
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
2f9c6b6  Trac #32214: unify dash in monoid repr

f7c1823  Merge branch 't/32214/termmonoidsunifyrepr' into t/32215/refactorelementconstructiontermmonoids

ba8efe6  Merge branch 'u/ghthhagelmayer/refactorelementconstructiontermmonoids' of git://trac.sagemath.org/sage into t/32215/refactorelementconstructiontermmonoids

51e035f  Trac #32215: fix doctests (due to merge of #32214)

d763d66  Trac #32215: full coverage of _element_constructor_

540d088  Merge commit 'c11b2f4' into t/32215/refactorelementconstructiontermmonoids

62cae10  Trac #32215: fix doctest after merge

a84a2f2  Merge branch 'u/dkrenn/refactorelementconstructiontermmonoids' of trac.sagemath.org:sage into t/32229/bterms_conversion

84ad7a3  Trac #32229: take absolute value of coefficient of BTerm

2474870  Merge branch 'u/behackl/bterms_conversion' of trac.sagemath.org:sage into t/32278/add_support_for_b_terms_in_asymptotic_ring

comment:19 Changed 9 months ago by
 Status changed from needs_review to positive_review
Merged latest version of dependencies.
comment:20 in reply to: ↑ 10 Changed 9 months ago by
Replying to behackl:
NotImplementedBZeroError
: Maybe add a straightforward test likesage: AR.<n> = AsymptoticRing('n^QQ', QQ) sage: AR(0).B(42) Traceback (most recent call last): ... NotImplementedBZero: got B(0) The error term B(0) means 0 for sufficiently large n.
Minor remark concerning this error message: "sufficiently large n" could be replaced by n >= 42
, because we actually know what "sufficiently large" means.
comment:21 followup: ↓ 22 Changed 6 months ago by
I'm still not sure what causes the patchbot failure. Would it make sense to move this to the latest beta?
comment:22 in reply to: ↑ 21 Changed 5 months ago by
Replying to behackl:
I'm still not sure what causes the patchbot failure. Would it make sense to move this to the latest beta?
A merge conflict has to be resolved, so moving to the latest beta is probably a good idea.
comment:23 Changed 5 months ago by
 Branch changed from u/behackl/add_support_for_b_terms_in_asymptotic_ring to 24748702f01276cdc678c969ff74482c3c8d347b
 Resolution set to fixed
 Status changed from positive_review to closed
I have implemented the initial functions for the usage of
Bterms
inAsymptoticRing
.I have three doctest errors:
The other two are the same from
src/sage/rings/asymptotic/asymptotic_ring.py", line 3379
andsrc/sage/rings/asymptotic/asymptotic_ring.py", line 3381
.Last 10 new commits:
Trac #31933. Item 65. Modify examples with explanation of valid_from to test the examples.
Trac #31933. Fix docbuild issue
Trac #31933 review: unify spelling Bterm
Trac #31933 review: simplfy doctests by using factory
Trac #31933 review: minor changes to docstrings etc
Trac #31933 Fix doctests after merge
Trac #31933. Item 67. Fix WARNINGBlock and Various formatting.
Trac #31933. Avoid trailing comma in latex repr.
Trac #31933. Item 65. Fix Examples for valid_from with explanation.
Trac #32278. Added initial support for Bterms. Doctests are failing for now