Opened 10 months ago
Closed 5 months ago
#32215 closed enhancement (fixed)
Asymptotic Term Monoids: refactor element construction
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  asymptotic expansions  Keywords:  gsoc2021 
Cc:  cheuberg, behackl, ghthhagelmayer  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  89ad9d2 (Commits, GitHub, GitLab)  Commit:  89ad9d2cbd4aed3a7c0b1065766c03727c754e01 
Dependencies:  #32214  Stopgaps: 
Change History (37)
comment:1 Changed 10 months ago by
 Branch set to u/dkrenn/refactorelementconstructiontermmonoids
comment:2 Changed 10 months ago by
 Commit set to c805fc19b60e9ac4ff028f63d4c687cb4d50bca2
 Dependencies set to #32214
 Status changed from new to needs_review
comment:3 Changed 10 months ago by
Review recommendation: commitwise
comment:4 Changed 10 months ago by
 Cc cheuberg behackl ghthhagelmayer added
comment:5 Changed 10 months ago by
 Commit changed from c805fc19b60e9ac4ff028f63d4c687cb4d50bca2 to f76b7e7ef42e8cf09301cc38277a11024e210f1d
Branch pushed to git repo; I updated commit sha1. New commits:
f76b7e7  Trac #32215: use defaults of super class

comment:6 Changed 10 months ago by
 Commit changed from f76b7e7ef42e8cf09301cc38277a11024e210f1d to c4792672602326a9cc615fb0060d5e09a20d69dd
comment:7 Changed 10 months ago by
 Commit changed from c4792672602326a9cc615fb0060d5e09a20d69dd to 3c5c0f0d88eec1853137f15466f254c378b32b5d
Branch pushed to git repo; I updated commit sha1. New commits:
3c5c0f0  fix coercion related to absorbtion

comment:8 Changed 10 months ago by
 Branch changed from u/dkrenn/refactorelementconstructiontermmonoids to u/behackl/refactorelementconstructiontermmonoids
comment:9 followups: ↓ 12 ↓ 15 Changed 10 months ago by
 Commit changed from 3c5c0f0d88eec1853137f15466f254c378b32b5d to 1975a10ac0f58240e195e66cf08c6105ad125a7e
 Reviewers set to Benjamin Hackl
 Status changed from needs_review to needs_work
Here are some minor points that should be addressed:
 The new exceptions caused by ambiguously passing additional
growth
andcoefficient
keyword arguments should be tested.
 Is there a particular reason for changing the implementation of
OTerm.can_absorb
?
 The implementations of
TermWithCoefficientMonoid._convert_construction_
andExactTermMonoid._convert_construction_
are identical. I think the tests contained in the docstring ofExactTermMonoid._convert_construction_
should be moved toTermWithCoefficientMonoid._convert_construction_
and the separate implementation ofExactTermMonoid._convert_construction_
should be removed.
Otherwise, the proposed changes look good to me. Here are the changes I've made in my reviewer commits:
 the
cached_method
import was unused and is now removed.
 The
EXAMPLES
block in some newly added methods usedTermMonoidFactory
to manually construct a factory, I've changed it tofrom sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid
(also see #32032).
 Language improvements for the error messages and the deprecation warning in
GenericTermMonoid._element_constructor
.
New commits:
bc7b2bc  removed superfluous import

d7cefd9  minor language improvements for docstrings and deprecation warning

1975a10  changed doctests of new methods to use DefaultTermMonoidFactory

comment:10 Changed 10 months ago by
 Branch changed from u/behackl/refactorelementconstructiontermmonoids to u/ghthhagelmayer/refactorelementconstructiontermmonoids
comment:11 Changed 10 months ago by
 Commit changed from 1975a10ac0f58240e195e66cf08c6105ad125a7e to 0a506354ea6ec01df3e7ba5b093d5c6e06179393
comment:12 in reply to: ↑ 9 Changed 10 months ago by
Replying to behackl:
Otherwise, the proposed changes look good to me. Here are the changes I've made in my reviewer commits:
 the
cached_method
import was unused and is now removed.
 The
EXAMPLES
block in some newly added methods usedTermMonoidFactory
to manually construct a factory, I've changed it tofrom sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid
(also see #32032).
 Language improvements for the error messages and the deprecation warning in
GenericTermMonoid._element_constructor
.
LGTM, thanks.
comment:13 Changed 10 months ago by
 Branch changed from u/ghthhagelmayer/refactorelementconstructiontermmonoids to u/dkrenn/refactorelementconstructiontermmonoids
comment:14 Changed 10 months ago by
 Commit changed from 0a506354ea6ec01df3e7ba5b093d5c6e06179393 to d763d663355c4519128a616fb5f6ce7ebba2cfee
Branch pushed to git repo; I updated commit sha1. New commits:
d763d66  Trac #32215: full coverage of _element_constructor_

comment:15 in reply to: ↑ 9 Changed 10 months ago by
Replying to behackl:
 The new exceptions caused by ambiguously passing additional
growth
andcoefficient
keyword arguments should be tested.
Tested now. Indeed there was code, that could have never been reached. I think we should have full coverage now.
 Is there a particular reason for changing the implementation of
OTerm.can_absorb
?
This was the only place, where the <=
of a term was used. However, I think it should be made clear what we compare here ("explicit is better than implicit"), so I switched to comparing the growth directly.
The reason for having <=
(and method le
) is for being compatible with SageMath's poset class in this perspective. If it wouldn't be for that, I would suggest to deprecate <=
in the long run. However, as there is a reason for keeping it, I suggest to, at least, not use it further.
 The implementations of
TermWithCoefficientMonoid._convert_construction_
andExactTermMonoid._convert_construction_
are identical. I think the tests contained in the docstring ofExactTermMonoid._convert_construction_
should be moved toTermWithCoefficientMonoid._convert_construction_
and the separate implementation ofExactTermMonoid._convert_construction_
should be removed.
I disagree here. Usually I am in strong favour of removeing code duplicities, however, _convert_construction_
is the one crucial method that all different terms (or their monoids, to be precise) have that do the conversion and making this conversion as explicit as possible helps understanding what is going on. I find it more of an incidient that two different type of terms have the same conversion.
comment:16 Changed 10 months ago by
 Status changed from needs_work to positive_review
I think we should have full coverage now.
Great, thanks!
I would suggest to deprecate
<=
in the long run. However, as there is a reason for keeping it, I suggest to, at least, not use it further.
Another example of spiritual deprecation. +1.
I find it more of an incidient that two different type of terms have the same conversion.
Okay, good argument.
This LGTM now, thanks for your efforts!
comment:17 Changed 10 months ago by
I am not sure why the deprecation_number
test in the most recent patchbot run failed though. The deprecation introduced here correctly uses 32215.
comment:18 Changed 10 months ago by
 Commit changed from d763d663355c4519128a616fb5f6ce7ebba2cfee to 540d08880bdcc5fbda1b2487bce9c74d3d35f27c
 Status changed from positive_review to needs_review
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
540d088  Merge commit 'c11b2f4' into t/32215/refactorelementconstructiontermmonoids

comment:19 Changed 10 months ago by
 Commit changed from 540d08880bdcc5fbda1b2487bce9c74d3d35f27c to 62cae10d33ab58abe5b2bbb2388adce8e01d1487
Branch pushed to git repo; I updated commit sha1. New commits:
62cae10  Trac #32215: fix doctest after merge

comment:20 Changed 10 months ago by
Merged #32153 and fixed doctest. It's now on 9.4.beta4.
comment:21 Changed 10 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:22 Changed 10 months ago by
 Status changed from needs_review to positive_review
comment:23 Changed 9 months ago by
 Keywords gsoc2021 added
comment:25 Changed 8 months ago by
 Branch changed from u/dkrenn/refactorelementconstructiontermmonoids to u/ghthhagelmayer/refactorelementconstructiontermmonoids
comment:26 Changed 8 months ago by
 Commit changed from 62cae10d33ab58abe5b2bbb2388adce8e01d1487 to d2eb45dcd993194e9e5c262a5ef3e3d352666a2d
 Status changed from needs_work to needs_review
comment:27 Changed 8 months ago by
 Commit changed from d2eb45dcd993194e9e5c262a5ef3e3d352666a2d to 5e9887771df00abbdf9827d8a46bf546721ac22b
Branch pushed to git repo; I updated commit sha1. New commits:
5e98877  Restore files which have been accidentally removed in the last commit and prevented building sage.

comment:28 Changed 6 months ago by
 Commit changed from 5e9887771df00abbdf9827d8a46bf546721ac22b to dfd5ae49c72effc3d0811f84d9e7a9d346809933
comment:29 followup: ↓ 30 Changed 6 months ago by
Somwthing went wrong with the last push here, so I went back to the status of u/dkrenn/refactorelementconstructiontermmonoids
and redid the merge and fixed the doctests. There is still one doctest failing:
File "src/sage/rings/asymptotic/term_monoid.py", line 4698, in sage.rings.asymptotic.term_monoid.BTerm.can_absorb Failed example: t2.absorb(t1) Expected: B(2003/400*x^3, x >= 20) Got: ... DeprecationWarning: Passing 'coefficient' as a positional argument is deprecated; specify it as keyword argument 'coefficient=...'. See https://trac.sagemath.org/32215 for details. B(2003/400*x^3, x >= 20)
comment:30 in reply to: ↑ 29 ; followup: ↓ 32 Changed 6 months ago by
Replying to ghthhagelmayer:
Somwthing went wrong with the last push here, so I went back to the status of
u/dkrenn/refactorelementconstructiontermmonoids
and redid the merge and fixed the doctests. There is still one doctest failing:File "src/sage/rings/asymptotic/term_monoid.py", line 4698, in sage.rings.asymptotic.term_monoid.BTerm.can_absorb Failed example: t2.absorb(t1) Expected: B(2003/400*x^3, x >= 20) Got: ... DeprecationWarning: Passing 'coefficient' as a positional argument is deprecated; specify it as keyword argument 'coefficient=...'. See https://trac.sagemath.org/32215 for details. B(2003/400*x^3, x >= 20)
Yes, this should be resolved as well: it comes from https://git.sagemath.org/sage.git/tree/src/sage/rings/asymptotic/term_monoid.py?id=683acc1eaa07fd3375bdd031516cfce30f1e363b#n4753, the coefficient should be passed via keyword argument.
comment:31 Changed 6 months ago by
 Commit changed from dfd5ae49c72effc3d0811f84d9e7a9d346809933 to 7e7b5c7bf93c713e3688a94ccb33561092556b83
Branch pushed to git repo; I updated commit sha1. New commits:
7e7b5c7  Trac #32215: fix a doctest. Use **kwds instead of coefficient

comment:32 in reply to: ↑ 30 Changed 6 months ago by
I fixed the doctest in 7e7b5c7
comment:33 followup: ↓ 35 Changed 6 months ago by
I've managed to find the problem causing the deprecation_number
plugin failure: looking at the implementation of the plugin over at https://github.com/sagemath/sagepatchbot/blob/745ec21b12f3bce03e8f1c8399cba0fb77bdc45c/sage_patchbot/plugins.py#L546, the plugin parses all added lines. If one of those contains deprecation(
or deprecated_function_alias(
, the code tries to parse the part following afterwards to find the ticket number, and then compares this parsed number to the actual ticket number.
The problem with our code is that we currently have
+ deprecation(
+ 32215,
+ "Passing 'coefficient' as a positional argument is deprecated; "
+ "specify it as keyword argument 'coefficient=...'.")
in our diff, so the code fails to properly parse the deprecation number. Changing this addition in term_monoid.py
to
+ deprecation(32215,
+ "Passing 'coefficient' as a positional argument is deprecated; "
+ "specify it as keyword argument 'coefficient=...'.")
will fix the failing test.
comment:34 Changed 6 months ago by
 Commit changed from 7e7b5c7bf93c713e3688a94ccb33561092556b83 to 89ad9d2cbd4aed3a7c0b1065766c03727c754e01
Branch pushed to git repo; I updated commit sha1. New commits:
89ad9d2  Trac #31125: fix deprecation plugin failure

comment:35 in reply to: ↑ 33 Changed 6 months ago by
Replying to behackl:
I've managed to find the problem causing the
deprecation_number
plugin failure: looking at the implementation of the plugin over at https://github.com/sagemath/sagepatchbot/blob/745ec21b12f3bce03e8f1c8399cba0fb77bdc45c/sage_patchbot/plugins.py#L546, the plugin parses all added lines. If one of those containsdeprecation(
ordeprecated_function_alias(
, the code tries to parse the part following afterwards to find the ticket number, and then compares this parsed number to the actual ticket number.The problem with our code is that we currently have
+ deprecation( + 32215, + "Passing 'coefficient' as a positional argument is deprecated; " + "specify it as keyword argument 'coefficient=...'.")
in our diff, so the code fails to properly parse the deprecation number. Changing this addition in
term_monoid.py
to+ deprecation(32215, + "Passing 'coefficient' as a positional argument is deprecated; " + "specify it as keyword argument 'coefficient=...'.")
will fix the failing test.
I changed that line accordingly.
comment:36 Changed 6 months ago by
 Status changed from needs_review to positive_review
This seems fine to me now, thank you! The latest patchbot test failures seem unrelated.
comment:37 Changed 5 months ago by
 Branch changed from u/ghthhagelmayer/refactorelementconstructiontermmonoids to 89ad9d2cbd4aed3a7c0b1065766c03727c754e01
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
add _repr_ for TermWithCoefficientMonoid
update and unify GenericTermMonoid _repr_
refactor element construction in term monoids
Trac #32215: deprecate positional "coefficient" argument