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: |
Description (last modified by )
Add support for B-terms 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/gh-thhagelmayer/add_support_for_b_terms_in_asymptotic_ring
comment:3 follow-up: ↓ 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 gh-thhagelmayer:
I have implemented the initial functions for the usage of
B-terms
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 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
|
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 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:
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
|
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 follow-ups: ↓ 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 follow-up: ↓ 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/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
- 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:
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/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
|
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 follow-up: ↓ 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
B-terms
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 B-term
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 WARNING-Block 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 B-terms. Doctests are failing for now