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: sage-9.5
Component: asymptotic expansions Keywords:
Cc: cheuberg, behackl, gh-thhagelmayer 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:

Status badges

Description


Change History (28)

comment:1 Changed 10 months ago by dkrenn

  • Branch set to u/dkrenn/bterms_conversion

comment:2 Changed 10 months ago by git

  • Commit set to 2660d4c5760dc4ce95a6adc0c3408b74d0a1e8d4

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

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

comment:3 follow-ups: Changed 10 months ago by dkrenn

5 failing doctests:

  1. FutureWarning (2 failing doctests), see review item 67 of #31933
  1. Negative coefficients in BTerms (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?
  1. Positional argument in failing doctests, see review item 73 of #31933.
Last edited 10 months ago by dkrenn (previous) (diff)

comment:4 Changed 10 months ago by dkrenn

  • Status changed from new to needs_review

comment:5 Changed 10 months ago by dkrenn

  • Authors set to Daniel Krenn
  • Cc cheuberg behackl gh-thhagelmayer added
  • Status changed from needs_review to needs_work

comment:6 Changed 10 months ago by dkrenn

  • Status changed from needs_work to needs_info

comment:7 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

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

comment:8 in reply to: ↑ 3 Changed 10 months ago by cheuberg

Replying to dkrenn:

  1. 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.

comment:9 Changed 10 months ago by gh-thhagelmayer

  • Branch changed from u/dkrenn/bterms_conversion to u/gh-thhagelmayer/bterms_conversion

comment:10 Changed 10 months ago by gh-thhagelmayer

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

9f49586Trac #31933. Item 77: move hint to a more meaningful line
c8c8ec4Trac #31933. Item 76: revert GenericGrowthElement._find_minimum_ Input explanation introduced in 1c1ebd41. Add missing empty line.
d076a0dTrac #31933. Item 83: remove unused import
343ddd8Trac #31933. Item 84: correct spelling
ff4747bTrac #31933. Item 86: make f-string an fr-string
f62e7edTrac #31933. Item 87: use the same description for MonomialGrowthElement._find_minimum_ as in GenericGrowthElement
832c80fTrac #31933. Item 88: remove 'the' in the acknowledgements to have consistency
2e46c20Trac #31933. Item 85: BTerm.__init__: Replace >= by \ge in all TeX formatted code
309d1e1Merge branch 't/31933/initial_support_for_bterms' into t/32229/bterms_conversion
9e5b3fbTrac #32229: fix doctests

comment:11 in reply to: ↑ 3 Changed 9 months ago by behackl

  • Reviewers set to Benjamin Hackl

Replying to dkrenn:

5 failing doctests:

  1. FutureWarning (2 failing doctests), see review item 67 of #31933
  1. Negative coefficients in BTerms (2 failing doctests). How to solve this? Is (simply) taking absolute values an option?
  1. 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).

Last edited 9 months ago by behackl (previous) (diff)

comment:12 Changed 9 months ago by gh-thhagelmayer

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

  • Branch changed from u/gh-thhagelmayer/bterms_conversion to u/behackl/bterms_conversion

comment:14 Changed 9 months ago by behackl

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

62330c0Merge branch 'u/gh-thhagelmayer/initial_support_for_bterms' of trac.sagemath.org:sage into t/32229/bterms_conversion
2f9c6b6Trac #32214: unify dash in monoid repr
f7c1823Merge branch 't/32214/term-monoids-unify-repr' into t/32215/refactor-element-construction-term-monoids
ba8efe6Merge branch 'u/gh-thhagelmayer/refactor-element-construction-term-monoids' of git://trac.sagemath.org/sage into t/32215/refactor-element-construction-term-monoids
51e035fTrac #32215: fix doctests (due to merge of #32214)
d763d66Trac #32215: full coverage of _element_constructor_
540d088Merge commit 'c11b2f4' into t/32215/refactor-element-construction-term-monoids
62cae10Trac #32215: fix doctest after merge
a84a2f2Merge branch 'u/dkrenn/refactor-element-construction-term-monoids' of trac.sagemath.org:sage into t/32229/bterms_conversion
84ad7a3Trac #32229: take absolute value of coefficient of BTerm

comment:15 Changed 9 months ago by gh-thhagelmayer

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

comment:16 follow-ups: Changed 9 months ago by 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?

comment:17 in reply to: ↑ 16 Changed 9 months ago by 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.

comment:18 in reply to: ↑ 16 Changed 9 months ago by behackl

  • 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/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!

comment:19 Changed 9 months ago by git

  • Commit changed from 84ad7a3efb761d10cbc0b02c4dd8145e8c991f04 to ffdfd69f79010eaa5d59b0981676662c6967f438

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

cc32a4bimplement multiplication for BTerm
ffdfd69let BTermMonoid coerce into OTermMonoid

comment:20 Changed 9 months ago by behackl

  • 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 BTermMonoids not coercing into OTermMonoids.

comment:21 Changed 8 months ago by 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!)

Last edited 8 months ago by mmezzarobba (previous) (diff)

comment:22 follow-up: Changed 7 months ago by 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?

comment:23 in reply to: ↑ 22 ; follow-up: Changed 7 months ago by gh-thhagelmayer

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

Replying to gh-thhagelmayer:

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

No problem at all--and thank you!

comment:25 Changed 6 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:26 Changed 5 months ago by git

  • Commit changed from ffdfd69f79010eaa5d59b0981676662c6967f438 to b29460f0a2bf90d646a15383a7e6d62309d9a9ba

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

6c3af69Merge commit 'f631489' into t/32215/refactor-element-construction-term-monoids
dfd5ae4Trac #32215: fix doctest after merge
7e7b5c7Trac #32215: fix a doctest. Use **kwds instead of coefficient
89ad9d2Trac #31125: fix deprecation plugin failure
b29460fMerge branch 'u/gh-thhagelmayer/refactor-element-construction-term-monoids' of trac.sagemath.org:sage into t/32229/bterms_conversion

comment:27 Changed 5 months ago by behackl

  • Status changed from needs_work to positive_review

Resolved conflict and tested locally, should be good now.

comment:28 Changed 5 months ago by vbraun

  • Branch changed from u/behackl/bterms_conversion to b29460f0a2bf90d646a15383a7e6d62309d9a9ba
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.