Opened 10 months ago

Closed 5 months ago

#32278 closed enhancement (fixed)

Add support for B-terms in Asymptotic Ring

Reported by: gh-thhagelmayer Owned by:
Priority: major Milestone: sage-9.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:

Status badges

Description (last modified by gh-thhagelmayer)

Add support for B-terms in Asymptotic Ring.

Change History (23)

comment:1 Changed 10 months ago by gh-thhagelmayer

  • Authors set to Thomas Hagelmayer
  • 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 gh-thhagelmayer

  • Branch set to u/gh-thhagelmayer/add_support_for_b_terms_in_asymptotic_ring

comment:3 follow-up: Changed 10 months ago by gh-thhagelmayer

  • Cc cheuberg behackl dkrenn added
  • Commit set to 5c5079df6021937e33f0a31f8768169e8128c21d
  • Dependencies set to #31933
  • Description modified (diff)
  • Status changed from new to needs_info

I have implemented the initial functions for the usage of B-terms in AsymptoticRing.

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):
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 718, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/doctest/forker.py", line 1137, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.rings.asymptotic.asymptotic_ring.AsymptoticRing.B[1]>", line 1, in <module>
        A.B(Integer(2)*x**Integer(3), {x: Integer(5)})
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/rings/asymptotic/asymptotic_ring.py", line 4694, in B
        return self.B(valid_from)
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/rings/asymptotic/asymptotic_ring.py", line 3400, in B
        return sum(self.parent().create_summand('B', growth=element, valid_from=valid_from)
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/rings/asymptotic/asymptotic_ring.py", line 3400, in <genexpr>
        return sum(self.parent().create_summand('B', growth=element, valid_from=valid_from)
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/rings/asymptotic/asymptotic_ring.py", line 4612, in create_summand
        return self(TM(data, **kwds), simplify=False, convert=False)
      File "sage/structure/parent.pyx", line 900, in sage.structure.parent.Parent.__call__ (build/cythonized/sage/structure/parent.c:9365)
        return mor._call_with_args(x, args, kwds)
      File "sage/structure/coerce_maps.pyx", line 180, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_with_args (build/cythonized/sage/structure/coerce_maps.c:5153)
        raise
      File "sage/structure/coerce_maps.pyx", line 170, in sage.structure.coerce_maps.DefaultConvertMap_unique._call_with_args (build/cythonized/sage/structure/coerce_maps.c:4943)
        return C._element_constructor(x, **kwds)
      File "/home/thhagelmayer/sagemath-trac-mirror/local/lib/python3.8/site-packages/sage/rings/asymptotic/term_monoid.py", line 1829, in _element_constructor_
        return self._create_element_(data.growth, data.coefficient)
    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 and src/sage/rings/asymptotic/asymptotic_ring.py", line 3381.


Last 10 new commits:

8e5e0e4Trac #31933. Item 65. Modify examples with explanation of valid_from to test the examples.
76d9bccTrac #31933. Fix docbuild issue
7e758a3Trac #31933 review: unify spelling B-term
786bd48Trac #31933 review: simplfy doctests by using factory
760a9dfTrac #31933 review: minor changes to docstrings etc
0afd7bfTrac #31933 Fix doctests after merge
33452c3Trac #31933. Item 67. Fix WARNING-Block and Various formatting.
bf9ff54Trac #31933. Avoid trailing comma in latex repr.
5a3c5b1Trac #31933. Item 65. Fix Examples for valid_from with explanation.
5c5079dTrac #32278. Added initial support for B-terms. Doctests are failing for now

comment:4 in reply to: ↑ 3 Changed 10 months ago by dkrenn

Replying to gh-thhagelmayer:

I have implemented the initial functions for the usage of B-terms in AsymptoticRing.

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 and src/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 git

  • Commit changed from 5c5079df6021937e33f0a31f8768169e8128c21d to 1e201eaf6b11e290b2d331cb8ba391ee1f65e5ce

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

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
766067fMerge branch 't/32229/bterms_conversion' into t/32278/add_support_for_b_terms_in_asymptotic_ring
1e201eaTrac #32278: fix doctests

comment:6 Changed 10 months ago by gh-thhagelmayer

  • Dependencies changed from #31933 to #31933, #32229
  • Milestone changed from sage-9.4 to sage-9.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:

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
766067fMerge branch 't/32229/bterms_conversion' into t/32278/add_support_for_b_terms_in_asymptotic_ring
1e201eaTrac #32278: fix doctests

comment:7 Changed 10 months ago by git

  • Commit changed from 1e201eaf6b11e290b2d331cb8ba391ee1f65e5ce to 6d9ea893fb05b0c56aa3a9308f0dfbc8f494d4fa

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

4798c10Trac #32278. moved valid_from logic from asymptotic_ring to term_monoid. corrected a doctest.
ca748e2Trac #32278: Fix doctests in misc.py
6d9ea89Trac #32278. moved logic from asymptoic_ring.py to term_monoid.py. fixed non working tests.

comment:8 Changed 10 months ago by gh-thhagelmayer

I moved the logic from asymptotic_ring to term_monoid and fixed the doctests.

comment:9 Changed 10 months ago by tscrim

  • Keywords gsoc2021 added; gsoc21 removed

comment:10 follow-ups: Changed 9 months ago by behackl

  • Status changed from needs_review to needs_work

This looks pretty functional already. Here are some remarks:

  1. AsymptoticExpansion.B: The tests in the docstring use AsymptoticRing.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 use AsymptoticExpansion.B.
  1. AsymptoticExpansion.B, AsymptoticRing.B, BTerm: The docstring, in particular: the description of valid_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.
  1. AsymptoticExpansion.B - Is there a particular reason that valid_from is just a positional argument, and not a keyword argument with default value 0 (like for AsymptoticRing.B?)
  1. NotImplementedBZeroError: Maybe add a straightforward test like
    sage: 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 git

  • Commit changed from 6d9ea893fb05b0c56aa3a9308f0dfbc8f494d4fa to 28c3984985d2ea7c1e1af1641d92eabe16f8d431

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

586ef6fTrac #32278. Item 4: add test for NotImplementedBZero Error
351d659Trac #32278. Item 1: add comment for indirect doctest
28c3984Trac #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 gh-thhagelmayer

  • Status changed from needs_work to needs_review

Replying to behackl:

Thank you for the comments.

  1. AsymptoticExpansion.B - Is there a particular reason that valid_from is just a positional argument, and not a keyword argument with default value 0 (like for AsymptoticRing.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 follow-up: Changed 9 months ago by behackl

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

  1. sage: (2*x).B({x: 20}) # indirect doctest -- this is actually not indirect, there you do use AsymptoticExpansion.B.
  1. "If you pass only a number as valid_from": I suggest to rephrase this as "If a number is passed to valid_from, ...".
  1. (2., continued): The docstring of BTerm should also be changed accordingly.

comment:14 Changed 9 months ago by git

  • Commit changed from 28c3984985d2ea7c1e1af1641d92eabe16f8d431 to 52b734d2d2c16435d9ce7fc1435ad830ad474ea9

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

1df00daTrac #32278. Items 6 and 7: correct docstring
52b734dTrac #32278. Item 5: remove "# indirect doctest"

comment:15 in reply to: ↑ 13 Changed 9 months ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review
  1. sage: (2*x).B({x: 20}) # indirect doctest -- this is actually not indirect, there you do use AsymptoticExpansion.B.
  1. "If you pass only a number as valid_from": I suggest to rephrase this as "If a number is passed to valid_from, ...".
  1. (2., continued): The docstring of BTerm should also be changed accordingly.

I have addressed these items.

comment:16 Changed 9 months ago by behackl

  • Branch changed from u/gh-thhagelmayer/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 behackl

  • 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 cross-check.

Otherwise, this looks good to me now, thank you.


New commits:

a57a275Trac #32278: fix singular/plural problem in docstrings

comment:18 Changed 9 months ago by git

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

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
2474870Merge 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 behackl

  • Status changed from needs_review to positive_review

Merged latest version of dependencies.

comment:20 in reply to: ↑ 10 Changed 9 months ago by cheuberg

Replying to behackl:

  1. NotImplementedBZeroError: Maybe add a straightforward test like
    sage: 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 follow-up: Changed 6 months ago by behackl

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 cheuberg

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 vbraun

  • 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
Note: See TracTickets for help on using tickets.