Asymptotic Ring BTerms: fix conversion

[git]

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

​3498449	Trac #31933 review: unify spelling B-term
​1933586	Trac #31933 review: simplfy doctests by using factory
​77717e4	Trac #31933 review: minor changes to docstrings etc
​2660d4c	Merge branch 't/31933/initial_support_for_bterms' into t/32229/bterms_conversion

[dkrenn]

5 failing doctests:

1. FutureWarning (2 failing doctests), see review item 67 of #31933

2. Negative coefficients in BTerms (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?

3. Positional argument in failing doctests, see review item 73 of #31933.

[mkoeppe]

Setting a new milestone for this ticket based on a cursory review.

[cheuberg]

Replying to dkrenn:

2. Negative coefficients in BTerms (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?

I think that negative coefficients should be forbidden for B-Terms.

[gh-thhagelmayer]

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 f-string an fr-string
​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

[behackl]

Replying to dkrenn:

5 failing doctests:

1. FutureWarning (2 failing doctests), see review item 67 of #31933

2. Negative coefficients in BTerms (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?

3. 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 B-Terms, 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).

[gh-thhagelmayer]

I have reviewed the code and it looks good to me as well.

[behackl]

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/gh-thhagelmayer/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/term-monoids-unify-repr' into t/32215/refactor-element-construction-term-monoids
​ba8efe6	Merge branch 'u/gh-thhagelmayer/refactor-element-construction-term-monoids' of git://trac.sagemath.org/sage into t/32215/refactor-element-construction-term-monoids
​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/refactor-element-construction-term-monoids
​62cae10	Trac #32215: fix doctest after merge
​a84a2f2	Merge branch 'u/dkrenn/refactor-element-construction-term-monoids' of trac.sagemath.org:sage into t/32229/bterms_conversion
​84ad7a3	Trac #32229: take absolute value of coefficient of BTerm

[gh-thhagelmayer]

I looked at the new changes. Looks good to me.

[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/site-packages/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?

[dkrenn]

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/site-packages/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.

[behackl]

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/site-packages/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!

[git]

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

​cc32a4b	implement multiplication for BTerm
​ffdfd69	let BTermMonoid coerce into OTermMonoid

[behackl]

Curiously, we simply had not implemented multiplication of BTerms so far. Now multiplication should respect the bounds; and I've also fixed BTermMonoids not coercing into OTermMonoids.

[mmezzarobba]

Also, I would expect absorption into B-terms to ignore non-growth factors in the absorbed term (and B-term creation to remove non-growth 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!)

[mmezzarobba]

I have opened #32692 for the point about non-growth 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?

[gh-thhagelmayer]

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.

[mmezzarobba]

Replying to gh-thhagelmayer:

Sorry for the inconvenience, there is no reason to delay this ticket.

No problem at all--and thank you!

[vbraun]

Merge conflict

[git]

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

​6c3af69	Merge commit 'f631489' into t/32215/refactor-element-construction-term-monoids
​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/gh-thhagelmayer/refactor-element-construction-term-monoids' of trac.sagemath.org:sage into t/32229/bterms_conversion

[behackl]

Resolved conflict and tested locally, should be good now.