Opened 10 months ago
Closed 5 months ago
#32229 closed defect (fixed)
Asymptotic Ring BTerms: fix conversion
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  asymptotic expansions  Keywords:  
Cc:  cheuberg, behackl, ghthhagelmayer  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl, Thomas Hagelmayer 
Report Upstream:  N/A  Work issues:  
Branch:  b29460f (Commits, GitHub, GitLab)  Commit:  b29460f0a2bf90d646a15383a7e6d62309d9a9ba 
Dependencies:  #31933, #32215  Stopgaps: 
Description
Change History (28)
comment:1 Changed 10 months ago by
 Branch set to u/dkrenn/bterms_conversion
comment:2 Changed 10 months ago by
 Commit set to 2660d4c5760dc4ce95a6adc0c3408b74d0a1e8d4
comment:3 followups: ↓ 8 ↓ 11 Changed 10 months ago by
comment:4 Changed 10 months ago by
 Status changed from new to needs_review
comment:5 Changed 10 months ago by
 Cc cheuberg behackl ghthhagelmayer added
 Status changed from needs_review to needs_work
comment:6 Changed 10 months ago by
 Status changed from needs_work to needs_info
comment:7 Changed 10 months ago by
 Milestone changed from sage9.4 to sage9.5
Setting a new milestone for this ticket based on a cursory review.
comment:8 in reply to: ↑ 3 Changed 10 months ago by
Replying to dkrenn:
 Negative coefficients in
BTerm
s (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?
I think that negative coefficients should be forbidden for BTerms.
comment:9 Changed 10 months ago by
 Branch changed from u/dkrenn/bterms_conversion to u/ghthhagelmayer/bterms_conversion
comment:10 Changed 10 months ago by
 Commit changed from 2660d4c5760dc4ce95a6adc0c3408b74d0a1e8d4 to 9e5b3fbdae06cd9ff787866df3ada169cbdb88d6
I merged tickets #32215 and #31933 which bumped the sage version to beta4. I also fixed the doctests which were failing after the merge.
Last 10 new commits:
9f49586  Trac #31933. Item 77: move hint to a more meaningful line

c8c8ec4  Trac #31933. Item 76: revert GenericGrowthElement._find_minimum_ Input explanation introduced in 1c1ebd41. Add missing empty line.

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

comment:11 in reply to: ↑ 3 Changed 9 months ago by
 Reviewers set to Benjamin Hackl
Replying to dkrenn:
5 failing doctests:
FutureWarning
(2 failing doctests), see review item 67 of #31933
 Negative coefficients in
BTerm
s (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?
 Positional argument in failing doctests, see review item 73 of #31933.
I believe that 1 and 3 are resolved in the meantime. Regarding 2 I also believe that upon creation of BTerms, we should take the absolute value of the specified coefficient. That should be relatively easy to take care of; do we just need to adapt BTerm.__init__
, or other places like BTermMonoid._convert_construction_
(as indicated by the added TODO) as well?
EDIT: forgot to say: I've reviewed the code here, the implementation looks good to me (and the fact that #32278 works as intended is also a good indicator of the code here doing what it should).
comment:12 Changed 9 months ago by
 Reviewers changed from Benjamin Hackl to Benjamin Hackl, Thomas Hagelmayer
I have reviewed the code and it looks good to me as well.
comment:13 Changed 9 months ago by
 Branch changed from u/ghthhagelmayer/bterms_conversion to u/behackl/bterms_conversion
comment:14 Changed 9 months ago by
 Commit changed from 9e5b3fbdae06cd9ff787866df3ada169cbdb88d6 to 84ad7a3efb761d10cbc0b02c4dd8145e8c991f04
 Status changed from needs_info to needs_review
I've merged the latest versions of #31933 and #32215, and then added another commit ensuring that we take the absolute value of the passed coefficient when constructing a BTerm. Needs review.
Last 10 new commits:
62330c0  Merge branch 'u/ghthhagelmayer/initial_support_for_bterms' of trac.sagemath.org:sage into t/32229/bterms_conversion

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

comment:15 Changed 9 months ago by
I looked at the new changes. Looks good to me.
comment:16 followups: ↓ 17 ↓ 18 Changed 9 months ago by
I have just started playing with BTerms. They are a nice addition, thank you all for your work!
The way the validity condition gets lost here looks like a bug to me:
sage: n = SR.var('n') sage: Asy = AsymptoticRing('QQ^n * n^ZZ', QQ) sage: ET = Asy.term_monoid('exact') sage: BT = Asy.term_monoid('B') /home/marc/co/sage/local/lib/python3.9/sitepackages/sage/structure/unique_representation.py:1007: FutureWarning: This class/method/function is marked as experimental. It, its functionality or its interface might change without a formal deprecation. See https://trac.sagemath.org/31922 for details. instance = typecall(cls, *args, **options) sage: bt = Asy(BT(n, valid_from={'n': 10})); bt B(1.000000000000000*n, n >= 10) sage: Asy(ET(2^n))*bt B(1.000000000000000*2^n*n, n >= 0)
(This is with 9.5.beta0 + branch from this ticket.) But maybe I am misunderstanding how BTerms are supposed to behave?
comment:17 in reply to: ↑ 16 Changed 9 months ago by
Replying to mmezzarobba:
I have just started playing with BTerms. They are a nice addition, thank you all for your work!
The way the validity condition gets lost here looks like a bug to me:
sage: n = SR.var('n') sage: Asy = AsymptoticRing('QQ^n * n^ZZ', QQ) sage: ET = Asy.term_monoid('exact') sage: BT = Asy.term_monoid('B') /home/marc/co/sage/local/lib/python3.9/sitepackages/sage/structure/unique_representation.py:1007: FutureWarning: This class/method/function is marked as experimental. It, its functionality or its interface might change without a formal deprecation. See https://trac.sagemath.org/31922 for details. instance = typecall(cls, *args, **options) sage: bt = Asy(BT(n, valid_from={'n': 10})); bt B(1.000000000000000*n, n >= 10) sage: Asy(ET(2^n))*bt B(1.000000000000000*2^n*n, n >= 0)(This is with 9.5.beta0 + branch from this ticket.) But maybe I am misunderstanding how BTerms are supposed to behave?
Indeed this is wrong. When I remember correctly, at the moment only polynomial terms are fully implemented (by #31933) and I would expect a NotImplementedError
for the above.
comment:18 in reply to: ↑ 16 Changed 9 months ago by
 Status changed from needs_review to needs_work
Replying to mmezzarobba:
I have just started playing with BTerms. They are a nice addition, thank you all for your work!
The way the validity condition gets lost here looks like a bug to me:
sage: n = SR.var('n') sage: Asy = AsymptoticRing('QQ^n * n^ZZ', QQ) sage: ET = Asy.term_monoid('exact') sage: BT = Asy.term_monoid('B') /home/marc/co/sage/local/lib/python3.9/sitepackages/sage/structure/unique_representation.py:1007: FutureWarning: This class/method/function is marked as experimental. It, its functionality or its interface might change without a formal deprecation. See https://trac.sagemath.org/31922 for details. instance = typecall(cls, *args, **options) sage: bt = Asy(BT(n, valid_from={'n': 10})); bt B(1.000000000000000*n, n >= 10) sage: Asy(ET(2^n))*bt B(1.000000000000000*2^n*n, n >= 0)(This is with 9.5.beta0 + branch from this ticket.) But maybe I am misunderstanding how BTerms are supposed to behave?
Hey! You are definitely right, multiplying BTerms apparently has some problems, I can reproduce it even in a simpler setting:
sage: AR.<n> = AsymptoticRing('n^QQ', QQ) sage: AR.B(42*n, valid_from=3) * AR.B(n, valid_from=5) B(42*n^2, n >= 0) sage: AR.B(42*n, valid_from=3) * n B(42*n^2, n >= 0)
I'll investigate where things go wrong. Thanks for checking it out!
comment:19 Changed 9 months ago by
 Commit changed from 84ad7a3efb761d10cbc0b02c4dd8145e8c991f04 to ffdfd69f79010eaa5d59b0981676662c6967f438
comment:20 Changed 9 months ago by
 Status changed from needs_work to needs_review
Curiously, we simply had not implemented multiplication of BTerms
so far. Now multiplication should respect the bounds; and I've also fixed BTermMonoid
s not coercing into OTermMonoid
s.
comment:21 Changed 8 months ago by
Also, I would expect absorption into Bterms to ignore nongrowth factors in the absorbed term (and Bterm creation to remove nongrowth factors, like it replaces the coefficient by its absolute value); this does not seem to be the case. (Not sure if this is something for this ticket, nor if I have already reported it!)
comment:22 followup: ↓ 23 Changed 7 months ago by
I have opened #32692 for the point about nongrowth factors raised in my last previous comment.
There are several comments above along the lines of "I have reviewed the code up to this point at it looks good to me". _I_ have reviewed the two commits made in response to my comment, and they look good to me, but I am reluctant to set the ticket to positive review without input from people actually working on asymptotic expansions :)
.
What do you think? It there a reason to delay this ticket any longer?
comment:23 in reply to: ↑ 22 ; followup: ↓ 24 Changed 7 months ago by
 Status changed from needs_review to positive_review
Replying to mmezzarobba:
What do you think? It there a reason to delay this ticket any longer?
Sorry for the inconvenience, there is no reason to delay this ticket.
I have reviewed the implementation of multiplication for BTerms
. LGTM.
comment:24 in reply to: ↑ 23 Changed 7 months ago by
Replying to ghthhagelmayer:
Sorry for the inconvenience, there is no reason to delay this ticket.
No problem at alland thank you!
comment:26 Changed 5 months ago by
 Commit changed from ffdfd69f79010eaa5d59b0981676662c6967f438 to b29460f0a2bf90d646a15383a7e6d62309d9a9ba
Branch pushed to git repo; I updated commit sha1. New commits:
6c3af69  Merge commit 'f631489' into t/32215/refactorelementconstructiontermmonoids

dfd5ae4  Trac #32215: fix doctest after merge

7e7b5c7  Trac #32215: fix a doctest. Use **kwds instead of coefficient

89ad9d2  Trac #31125: fix deprecation plugin failure

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

comment:27 Changed 5 months ago by
 Status changed from needs_work to positive_review
Resolved conflict and tested locally, should be good now.
comment:28 Changed 5 months ago by
 Branch changed from u/behackl/bterms_conversion to b29460f0a2bf90d646a15383a7e6d62309d9a9ba
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Trac #31933 review: unify spelling Bterm
Trac #31933 review: simplfy doctests by using factory
Trac #31933 review: minor changes to docstrings etc
Merge branch 't/31933/initial_support_for_bterms' into t/32229/bterms_conversion