Opened 8 months ago
Closed 5 months ago
#31933 closed enhancement (fixed)
Initial support for BTerms
Reported by:  ghthhagelmayer  Owned by:  ghthhagelmayer 

Priority:  major  Milestone:  sage9.5 
Component:  asymptotic expansions  Keywords:  gsoc2021, asymptotics 
Cc:  cheuberg, behackl  Merged in:  
Authors:  Thomas Hagelmayer  Reviewers:  Daniel Krenn, Clemens Heuberger, Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  f631489 (Commits, GitHub, GitLab)  Commit:  f631489e308d859ee22ff2464cc28724f1789782 
Dependencies:  #32153  Stopgaps: 
Description (last modified by )
Adding initial support for BTerms implementing the absorb() and can_absorb() functions.
Change History (69)
comment:1 Changed 8 months ago by
 Cc cheuberg behackl added
 Component changed from PLEASE CHANGE to asymptotic expansions
 Description modified (diff)
 Keywords gsoc21 asymptotics added
 Owner changed from (none) to ghthhagelmayer
 Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 8 months ago by
 Branch set to u/ghthhagelmayer/initial_support_for_bterms
comment:3 Changed 8 months ago by
 Commit set to 463bcba2278887d598218a4660b62d38989e63a8
comment:4 Changed 8 months ago by
 Commit changed from 463bcba2278887d598218a4660b62d38989e63a8 to 8b40bf9b1513c36ed7b18f249164232ec213fa1e
Branch pushed to git repo; I updated commit sha1. New commits:
8b40bf9  fix typo in class BTerm doctest

comment:5 Changed 7 months ago by
 Commit changed from 8b40bf9b1513c36ed7b18f249164232ec213fa1e to 0f0b4d93a2f4b46ae720a40e79be61d57639d5b3
Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:
0e89713  changed strings in class BTerm to fstrings

809ce70  Add representation to classes BTerm and BTermMonoid. Change super(BTerm, self) to super() in __init__

f78de5e  added descriptions

378165d  make represenation more user friendly

a7739e9  refactor can_absorb and _absorb_ functions

99851f2  fix BTermMonoid representation string

0a8d14e  add description to can_absorb

d4c2609  fix can_absorb() function

aeae824  fix absorb function and doctest

0f0b4d9  fix typo in class BTerm doctest

comment:6 followup: ↓ 7 Changed 7 months ago by
Just some comments that got to me when scrolling through the current code (well being aware that it is work in progress):
 Consider passing
**kwds
instead ofvalid_form
in+ def _element_constructor_(self, data, coefficient=None, valid_from=None):
 When I got it correct what the code does, a more Pythonic way of writing
+ valid_from_string = '' + for key in self.valid_from: + if key is list(self.valid_from)[1]: + valid_from_string += f'{key} >= {self.valid_from[key]}' + else: + valid_from_string += f'{key} >= {self.valid_from[key]} and '
would be something like
valid_from_string = ' and '.join(f'{key} >= {value}' for key, value in self.valid_from.items())
Maybe even use more describing names than key
and value
(not checked what is used in the code elsewhere)
 Remove empty line in
_create_element_
before thereturn
.
 More like a question, why was the
+
moved up in return (self._log_coefficient_(base=base, locals=locals)  + self._log_growth_(base=base, locals=locals)) + return (self._log_coefficient_(base=base, locals=locals) + + self._log_growth_(base=base, locals=locals))
?
comment:7 in reply to: ↑ 6 Changed 7 months ago by
Replying to dkrenn:
Thank you for your suggestions!
 More like a question, why was the
+
moved up
I must have accidentally touched that line. This was not intended. I will move it back down.
comment:8 Changed 7 months ago by
Just some comments from my side at this point:
BTerm.__init__
: Please add tests for all possible exceptions which may be raised in this method
./sage docbuild includetestsblocks underscore reference/asymptotic html
gave a warning:local/lib/python3.9/sitepackages/sage/rings/asymptotic/term_monoid.py:docstring of sage.rings.asymptotic.term_monoid.BTerm._absorb_:9: WARNING: Literal block expected; none found.
The cause seems to be a missing indentation of the content of theEXAMPLES
block.
TermMonoidFactory.create_key_and_extra_args
: I would prefer to write "has to be either 'exact', 'O', 'B' or an instance" in theValueError
(commas instead of repeated "or").
 example for
valid_from
BTerm.__init__
: description of the parametervalid_from
: "vriable" > "variable", "the bound by this term" > "The bound implied by this term".
BTerm._repr_
,BTermMonoid._repr_
: I suggest to replace the\
and+ \
at the ends of the lines by parentheses, see https://docs.python.org/3/reference/lexical_analysis.html#stringliteralconcatenation
 The developer guide (https://doc.sagemath.org/html/en/developer/coding_basics.html) does not include closing punctuation in INPUT/OUTPUT blocks, so please remove these in new code (e.g.
BTerm.can_absorb
,BTerm._absorb_
)
BTerm._repr_
, last example: I think thatBT_QQ(x, 3, {'x': 20, 'm': 5})
should raise an error becausem
is not a valid variable understood by the growth groupG
.
BTerm.can_absorb
, docstring: "An BTerm can absorb" > "A BTerm can absorb"
BTerm.can_absorb
, docstring: "with weaker or equal growth", change the code accordingly, and add examples for absorption of BTerms of different growths.
BTerm.can_absorb
: I think that we should discuss whethert2.can_absorb(t1)
should beTrue
orFalse
; more specifically, whether it is allowable to changevalid_from
during absorption. It should also be discussed whether we want to use different parents for different values invalid_from
.
BTerm.can_absorb
: TheNotImplementedError
seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys ofvalid_from
must agree, which should be checked somewhere).
BTerm._absorb_
: I think that neither the example nor the code are correct. Dividing byself.valid_from[str(self.growth)]
is only valid for situations where the exponents ofx
differ by exactly one. And for other growth groups, this needs more attention (I think that computing the correct factor could be moved to a method of the growth elements).
BTerm._create_element_
: EmptyINPUT
,TESTS
sections, missingOUTPUT
section.
TermMonoidFactory
docstring: replace.
by,
afterExactTermMonoid
.
TermMonoidFactory
docstring: I think that "(capital letterB
)" can be removed, there is no danger of confusion here (in contrast toO
Terms where the capital letterO
might be confused with the digit0
).
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms.
comment:9 Changed 7 months ago by
 Commit changed from 0f0b4d93a2f4b46ae720a40e79be61d57639d5b3 to 4060b6c083ddf6806cedf682d746457687321f34
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
7be8d49  Move binary operator back down after accidentally moving it up

1f698d3  Passing **kwds in _element_constructor_ instead of valid_from. Adjusting code to be compatible with changes

5d9337b  fix indentation of EXAMPLES block in absorb()

0eeb50d  Added check in init. All defined variables in valid_from have to occur in the term.

3ec4a6a  Implemented find_minimum() in growth_group.py.

ee337e2  added tests to init from BTerm

8e754d7  replacing '\' and '+ \' with parenthesis' in BTerm._repr_ and BTermMonoid._repr_

9c7771e  replaced 'or' with ',' in ValueError text in create_key_and_extra_args

b5f1470  Small corrections in dosctrings. Correcting typos and removing trailing dots.

4060b6c  Remove unused example from BTerm.can_absrob()

comment:10 Changed 7 months ago by
I had a look on the current version (4060b6c). If I am not mistaken, then items
8.
14.
15.
16.
18.
19.
20.
21.
from above are still open at this time.
I have a few other comments:
 Missing empty line after
WARNING::
leads to nonproper formatting of the warning.
 The link to
sage.misc.superseded.experimental
is not formatted properly.
 Followup to 12: I think that the keys of
valid_from
should at least contain the variables of the actual growth element and at most contain the variables of the growth group. The new check introduced in 0eeb50da71 means that the keys ofvalid_from
are enforced to at most contain the variables of the actual growth element.
BTerm._absorb_
should not inspectself.growth.exponent
orother.growth.exponent
: This only works forMonomialGrowthElement
. Instead,self.growth
should be compared toother.growth
(comparison of growth elements is implemented in the growth group which will know what to do).
._element_constructor_
: There is now antry
/except
/else
construct where theelse
branch could be moved outside the construct (as it was the case before this ticket; introducing the**kwds
parameter should not really change the structure here).
BTerm._repr_
is now untested (since 0eeb50da71) in the case of several entries ofvalues_from
BTerm._absorb_
: Computation ofvalid_from_new
will always yield a onekey dictionary even if the original dictionaries have more than one key. I think that a loop over the union of the keys of the two original dictionaries should be done; if a key is in both dictionaries, then take the maximum (as is the case now), otherwise take the unique entry.
can_absorb
: Missing empty line afterTESTS::
leads to unproper formatting of tests (when using./sage docbuild includetestsblocks
)
can_absorb
: I think that absorption should only happen ifself.growth >= other.growth
. In particular,B(x^3)
can absorbB(x^2)
(because it is larger), butB(x^2)
should not absorbB(x^3)
.
GenericGrowthElement._find_minimum_
: please add docstring.
GenericGrowthElement._find_minimum_
andMonomialGrowthElement._find_minimum_
: I think that it would be simpler not to haveother
in the signature of this method. If B(g) absorbs B(h) for some g >= h, thenabsorb
could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then callq._find_minimum_
.
MonomialGrowthElement._find_minimum_
: Currently, this is symmetric inself
andother
. That should not be the case:self
must be at leastother
in order to allow for absorption. After 32., this should simply be a check whetherself
is at least1
. (see 30)
MonomialGrowthElement._find_minimum_
: I think that the signature should be the same as forGenericGrowthElement._find_minimum_
: the last parameter should bevalid_from
instead ofvalid_from_bound
. I think that it should be the growth element's task to extract the correct variable.
MonomialGrowthElement._find_minimum_
: I suggest the following onesentence description: "Find the minimum of this growth element over the range implied byvalid_from
"
BTerm._absorb_
: I think thatt2
should not absorbt1
(related to 30 and 33).
comment:11 Changed 7 months ago by
Please send a short message or reply to the raised items once your branch is ready to be reviewed again. I agree with the points raised by Clemens; in particular when it comes to the comments concerning _find_minimum_
(31 – 35).
Regarding 25, and BTerm._repr_
in general: this output will have to be adapted once BTerm
s are made available for the asymptotic ring so that they can be represented by shorter notation that is suitable for representation in a sum together with other terms in an AsymptoticExpansion
(the expansions use the repr
methods of the terms). I think it would be fine to delay ironing out this representation to the future ticket integrating BTerm
s with AsymptoticRing
, but that also means I wouldn't put too much effort into polishing doctests using the current representation.
Some further minor comments:
 In the spirit of #32032, please replace the lines
sage: from sage.rings.asymptotic.term_monoid import TermMonoidFactory sage: TermMonoid = TermMonoidFactory('__main__.TermMonoid')
in the doctests you are adding here withsage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid
 The onesentence description for
MonomialGrowthElement._find_minimum_
suggested by Clemens above also works forGenericGrowthElement._find_minimum_
.
BTerm.can_absorb
: In the NOTE block of the docstring, the second occurrence ofBTerm
should also be a reference. Also, make sure to add a newline afterTESTS::
.
comment:12 followup: ↓ 13 Changed 7 months ago by
 Maybe, in
 return self._create_element_(data, coefficient) + else: + return self._create_element_(data, coefficient, **kwds)
remove the else
block of the try
and simply return as before. The else
block of a try
is useful if there is a finally
block because it is executed before this finally
block.
Also, all other code in the asymptotic package does not use it, so I suggest to be consistent. One reason for this choice is, that there is a clear, nonindented ending (the return
above) of an indented block.
comment:13 in reply to: ↑ 12 Changed 7 months ago by
Replying to dkrenn:
 Maybe, in
 return self._create_element_(data, coefficient) + else: + return self._create_element_(data, coefficient, **kwds)remove the
else
block of thetry
and simply return as before. Theelse
block of atry
is useful if there is afinally
block because it is executed before thisfinally
block.Also, all other code in the asymptotic package does not use it, so I suggest to be consistent. One reason for this choice is, that there is a clear, nonindented ending (the
return
above) of an indented block.
I think this is 26 (but with more explanations). :)
comment:14 Changed 7 months ago by
 Commit changed from 4060b6c083ddf6806cedf682d746457687321f34 to 9fffc5a1de1b0b89208b34de3866b5034ad444a3
Branch pushed to git repo; I updated commit sha1. New commits:
29cc561  Item 26 and 40. Removing else of the tryblock and returning as before

3e1f74f  Items 22, 29, 39. Added missing empty lines after WARNING and TEST. BTerm.can_absorb() NOTES block, fixed BTerm formatting.

9fffc5a  Item 19 and 20. Fix TermMonoidFactory docstring.

comment:15 Changed 7 months ago by
 Commit changed from 9fffc5a1de1b0b89208b34de3866b5034ad444a3 to 5961ac9b4e36f08674c6bc6db8091dfd2430f2eb
Branch pushed to git repo; I updated commit sha1. New commits:
5961ac9  added example to BTerm._absorb_ where absorbtion fails.

comment:16 followup: ↓ 18 Changed 7 months ago by
Items worked on and considered done by the latest commits: 14, 15, 19, 20, 22, 26, 29, 36, 39, 40.
Commit https://git.sagemath.org/sage.git/commit/?id=9fffc5a1de1b0b89208b34de3866b5034ad444a3 contains items 14 and 15 aswell. I forgot to make a seperate commit for those two items. I will be more cautious from now on.
I have a question about number
23 The link to sage.misc.superseded.experimental is not formatted properly.
What is the problem with the format? I have built the docs and can't see any issue.
comment:17 Changed 7 months ago by
 Status changed from new to needs_review
comment:18 in reply to: ↑ 16 Changed 7 months ago by
 Status changed from needs_review to needs_work
Replying to ghthhagelmayer:
Items worked on and considered done by the latest commits: 14, 15, 19, 20, 22, 26, 29, 36, 39, 40.
I do not fully agree with 14: the code has been fixed as proposed in 14, but the doc string does not yet reflect this.
I have a question about number
23 The link to sage.misc.superseded.experimental is not formatted properly.What is the problem with the format? I have built the docs and can't see any issue.
For me, it looks as follows: In other words, it is not a link.
As far as I am aware, the following items are open from previous rounds:
 example for
valid_from
BTerm.can_absorb
, docstring: "with weaker or equal growth" (rest of the former 14 is done)
BTerm.can_absorb
: TheNotImplementedError
seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys ofvalid_from
must agree, which should be checked somewhere).
BTerm._create_element_
: EmptyINPUT
,TESTS
sections, missingOUTPUT
section.
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms.
 The link to
sage.misc.superseded.experimental
is not formatted properly.
 Followup to 12: I think that the keys of
valid_from
should at least contain the variables of the actual growth element and at most contain the variables of the growth group. The new check introduced in 0eeb50da71 means that the keys ofvalid_from
are enforced to at most contain the variables of the actual growth element.
BTerm._absorb_
should not inspectself.growth.exponent
orother.growth.exponent
: This only works forMonomialGrowthElement
. Instead,self.growth
should be compared toother.growth
(comparison of growth elements is implemented in the growth group which will know what to do).
BTerm._repr_
is now untested (since 0eeb50da71) in the case of several entries ofvalues_from
BTerm._absorb_
: Computation ofvalid_from_new
will always yield a onekey dictionary even if the original dictionaries have more than one key. I think that a loop over the union of the keys of the two original dictionaries should be done; if a key is in both dictionaries, then take the maximum (as is the case now), otherwise take the unique entry.
GenericGrowthElement._find_minimum_
: please add docstring.
GenericGrowthElement._find_minimum_
andMonomialGrowthElement._find_minimum_
: I think that it would be simpler not to haveother
in the signature of this method. If B(g) absorbs B(h) for some g >= h, thenabsorb
could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then callq._find_minimum_
.
MonomialGrowthElement._find_minimum_
: Currently, this is symmetric inself
andother
. That should not be the case:self
must be at leastother
in order to allow for absorption. After 32., this should simply be a check whetherself
is at least1
. (see 30)
MonomialGrowthElement._find_minimum_
: I think that the signature should be the same as forGenericGrowthElement._find_minimum_
: the last parameter should bevalid_from
instead ofvalid_from_bound
. I think that it should be the growth element's task to extract the correct variable.
MonomialGrowthElement._find_minimum_
: I suggest the following onesentence description: "Find the minimum of this growth element over the range implied byvalid_from
"
 The onesentence description for
MonomialGrowthElement._find_minimum_
suggested by Clemens above also works forGenericGrowthElement._find_minimum_
.
A few new items (some of them not related to the changes, but new findings in previously pushed code, sorry):
BTerm.can_absorb
, docstring, onesentence description: replace`BTerm`
by``BTerm``
BTerm.can_absorb
: The codeif self.growth >= other.growth: return True else: return False
could be simplified to
return self.growth >= other.growth
.
BTerm.can_absorb
: I fear I forgot to mention the following in the last round: I do not think that the restrictionif key in other.valid_from.keys()
is appropriate: we intend to fixvalid_from
during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
BTerm.__init__
: missing empty line afterTESTS::
.
comment:19 followup: ↓ 20 Changed 7 months ago by
 Commit changed from 5961ac9b4e36f08674c6bc6db8091dfd2430f2eb to 41b1974d36432460fc45a63d390c7e088fee46cc
Branch pushed to git repo; I updated commit sha1. New commits:
218d166  Added example for multivariate BTerm in BTerm._init_

5dad24a  Item 18 BTerm._create_element_ Filled out INPUT and TESTS section, added OUTPUT section.

d4b5f99  Item 23. Formatted warning properly so that sage.misc.superseded.experimental is a link.

365971d  Item 44. BTerm._init_ addd empty line after TESTS.

6a8d7cf  Item 41. Fix docstring.

41b1974  Item 14. Fix BTerm.can_absorb docstring.

comment:20 in reply to: ↑ 19 Changed 7 months ago by
 The test you have added in 218d166 should assign x and y as the variables of the growth group
B
, likesage: x, y = B('x'), B('y')
as the
NameError
currently thrown has nothing to do with the implementation.
 (18. cont.) In
BTerm._create_element_
, the description of**kwds
should just say that these are additional keyword arguments. I think you could even replace**kwds
by the explicit keyword you are expecting there, namelyvalid_from
. The general form,**kwds
is mainly important for the generic_element_constructor_
; after that we know which arguments we can expect.
From my point of view, points 14, 18, 41, 44 are resolved (or replaced by a followup). 23 also looks good to me, but I didn't look at the built documentation.
comment:21 Changed 7 months ago by
23: This does not yet render properly:
As far as I am aware, the following items are open from previous rounds:
 example for
valid_from
: It would be great to have an example where the parametervalid_from
is explained within this concrete situation.
BTerm.can_absorb
: TheNotImplementedError
seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys ofvalid_from
must agree, which should be checked somewhere).
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms.
 The link to
sage.misc.superseded.experimental
is not formatted properly.
 Followup to 12: I think that the keys of
valid_from
should at least contain the variables of the actual growth element and at most contain the variables of the growth group. The new check introduced in 0eeb50da71 means that the keys ofvalid_from
are enforced to at most contain the variables of the actual growth element.
BTerm._absorb_
should not inspectself.growth.exponent
orother.growth.exponent
: This only works forMonomialGrowthElement
. Instead,self.growth
should be compared toother.growth
(comparison of growth elements is implemented in the growth group which will know what to do).
BTerm._repr_
is now untested (since 0eeb50da71) in the case of several entries ofvalues_from
BTerm._absorb_
: Computation ofvalid_from_new
will always yield a onekey dictionary even if the original dictionaries have more than one key. I think that a loop over the union of the keys of the two original dictionaries should be done; if a key is in both dictionaries, then take the maximum (as is the case now), otherwise take the unique entry.
GenericGrowthElement._find_minimum_
: please add docstring.
GenericGrowthElement._find_minimum_
andMonomialGrowthElement._find_minimum_
: I think that it would be simpler not to haveother
in the signature of this method. If B(g) absorbs B(h) for some g >= h, thenabsorb
could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then callq._find_minimum_
.
MonomialGrowthElement._find_minimum_
: Currently, this is symmetric inself
andother
. That should not be the case:self
must be at leastother
in order to allow for absorption. After 32., this should simply be a check whetherself
is at least1
. (see 30)
MonomialGrowthElement._find_minimum_
: I think that the signature should be the same as forGenericGrowthElement._find_minimum_
: the last parameter should bevalid_from
instead ofvalid_from_bound
. I think that it should be the growth element's task to extract the correct variable.
MonomialGrowthElement._find_minimum_
: I suggest the following onesentence description: "Find the minimum of this growth element over the range implied byvalid_from
"
 The onesentence description for
MonomialGrowthElement._find_minimum_
suggested by Clemens above also works forGenericGrowthElement._find_minimum_
.
BTerm.can_absorb
: The codeif self.growth >= other.growth: return True else: return False
could be simplified to
return self.growth >= other.growth
.
BTerm.can_absorb
: I fear I forgot to mention the following in the last round: I do not think that the restrictionif key in other.valid_from.keys()
is appropriate: we intend to fixvalid_from
during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
 The test you have added in 218d166 should assign
x
andy
as the variables of the growth groupB
, likesage: x, y = B('x'), B('y')
as the
NameError
currently thrown has nothing to do with the implementation.
 (18. cont.) In
BTerm._create_element_
, the description of**kwds
should just say that these are additional keyword arguments. I think you could even replace**kwds
by the explicit keyword you are expecting there, namelyvalid_from
. The general form,**kwds
is mainly important for the generic_element_constructor_
; after that we know which arguments we can expect.
New items:
BTermMonoid
description of input variablecoefficient_ring
: Remove trailing full stop.
BTermMonoid._create_element_
: ReplaceTESTS:
byTESTS::
.
comment:22 Changed 7 months ago by
 Commit changed from 41b1974d36432460fc45a63d390c7e088fee46cc to 78c408f9b6f7215e7a38aad23bcbd6cfd925f097
Branch pushed to git repo; I updated commit sha1. New commits:
5a3429b  Items 25, 28, 31, 32, 34, 35, 38. Rewriting BTerm._absorb_, MonomialGrowthElement._find_minimum_ and GenericGrowthElement._find_minimum_

cecb744  Item 45. Correction of the multivariate doctest

e6c19f2  Items 24 and 43. BTerm.can_absorb: be more lenient on variable checking

6e64780  Item 18 cont. BTermMonoid._create_element_: rename **kwds to valid_from

78c408f  Item 27. BTerm._repr_: Add doctest for the case of multiple valid_from values

comment:23 Changed 7 months ago by
 Status changed from needs_work to needs_review
With the latest commit I consider following items to be open:
 example for
valid_from
BTerm.can_absorb
: TheNotImplementedError
seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys ofvalid_from
must agree, which should be checked somewhere).
BTerm.can_absorb
: I think that BTerms should be able to absorb exact terms.
MonomialGrowthElement._find_minimum_
: Currently, this is symmetric in self and other. That should not be the case: self must be at least other in order to allow for absorption. After 32., this should simply be a check whether self is at least 1. (see 30)
comment:24 Changed 7 months ago by
 Concerning 24: Assume we have
sage: B = GrowthGroup('x^ZZ * y^ZZ'); sage: x, y = B('x'), B('y') sage: BT_ZZ = BTermMonoid(TermMonoid, B, ZZ)
Then
sage: BT_ZZ(x^3, 42, valid_from={'x': 10}) sage: BT_ZZ(x^3, 42, valid_from={'x': 10, 'y': 20})should be valid (because the term only contains
x
, but the corresponding growth group containsx
andy
),sage: BT_ZZ(x^3*y^2, 42, valid_from={'x': 10})should be invalid because the term contains
x
andy
, buty
is missing invalid_from
and finallysage: BT_ZZ(x^3, 42, valid_from={'x': 10, 'z': 20})should be forbidden (because the variable
z
is not one of the variables of the growth group).
I do not think that this is the case now (and if it is, then please add the tests above), so I consider 24 to be still open.
 I consider 27 to be still open: I think that there should be a successful multivariate test which actually tests
_repr_
instead of a test which fails much earlier. See one of the examples mentioned above for 24 for possible candidates for tests.
 I consider 43 to be still open: the lines
for variable in self.variable_names(): if variable in self.valid_from.keys():
should not be there because it is not the task of
can_absorb
to deal withvalid_from
at all.
 Items 23, 47, 48 are not mentioned in your commits and I think that they are still open.
Thus as far as I am concerned, the following items are open from previous rounds:
 example for
valid_from
: It would be great to have an example where the parametervalid_from
is explained within this concrete situation.
BTerm.can_absorb
: TheNotImplementedError
seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys ofvalid_from
must agree, which should be checked somewhere).
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms.
 The link to
sage.misc.superseded.experimental
is not formatted properly.
 Followup to 12: I think that the keys of
valid_from
should at least contain the variables of the actual growth element and at most contain the variables of the growth group. The new check introduced in 0eeb50da71 means that the keys ofvalid_from
are enforced to at most contain the variables of the actual growth element.
BTerm._repr_
is now untested (since 0eeb50da71) in the case of several entries ofvalues_from
BTerm.can_absorb
: I fear I forgot to mention the following in the last round: I do not think that the restrictionif key in other.valid_from.keys()
is appropriate: we intend to fixvalid_from
during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
BTermMonoid
description of input variablecoefficient_ring
: Remove trailing full stop.
BTermMonoid._create_element_
: ReplaceTESTS:
byTESTS::
.
New items:
GenericGrowthElement._find_minimum_
: replace""""
by"""
at the beginning of the docstring
GenericGrowthElement._find_minimum_
,MonomialGrowthElement._find_minimum_
: I suggest to change the description ofvalid_from
to "a dictionary describing the range of the minimization: the keys are names of variables and the range is the intersection over the ranges where the absolute value of the variable designated by the key is at least the corresponding value"
GenericGrowthElement._find_minimum_
: remove trailing full stop from output section.
GenericGrowthElement._find_minimum_
,MonomialGrowthElement._find_minimum_
: Add full stop to onesentence description.
MonomialGrowthElement._find_minimum_
: replacenext(iter(valid_from.values()))
by something resembling tovalid_from[self.variable_names()[0]]
(with some extra care whenvariable_names()
is the empty tuple).
 (33. ctd)
MonomialGrowthElement._find_minimum_
: ifself._is_lt_one_()
, then an exception (perhapsDecreasingGrowthElementError
derived fromValueError
) should be raised.
BTerm.absorb
: I suggest to replaceself.growth._lt_(other.growth)
byself.growth < other.growth
: the operator<
is overloaded in terms of the_lt_
method, so it ultimately boils down to the same code, but using<
makes it more pythonic and more readable.
BTerm.absorb
: I suggest to replacegrowth_new = max(self.growth, other.growth)
bygrowth_new = self.growth
(because this is ensured anyway) or to remove the variablegrowth_new
completely.
BTerm.absorb
: I think that the variable namescoeff_max
andcoeff_min
are not very clear; so I proposed to remove them completely, they can be replaced byself.coefficient
andother.coefficient
, respectively.
GenericGrowthElement._find_minimum_
,MonomialGrowthElement._find_minimum_
: add at least one test (all functions in SageMath must be tested)
comment:25 Changed 7 months ago by
I will have a look at code of this ticket once the items above are processed; no comments at the moment to these items.
comment:26 Changed 6 months ago by
 Commit changed from 78c408f9b6f7215e7a38aad23bcbd6cfd925f097 to 06128c1aee93d70470bf47971cb8f68875ce6947
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
d4cfa7c  Item 47 and 48. Remove trailing full stop in BTermMonoid description and replace TESTS: with TESTS:: int BTermMonoid._create_element_

4666a16  Items 49, 50, 51, 52. GenericGrowthElement._find_minimum_: replace """" by """ at the beginning of the docstring, new valid_from description, removed trailing full stop from output, added full stop to onesentence description. MonomialGrowthElement._find_minimum_: new valid_from description, added full stop to onesentence description

7109144  Items 55, 56, 57. Replaced self.grwoth._lt_(other.growth) by self.growth < other.growth. Removed variables coeff_max, coeff_min and growth_new and replaced them with self.coefficient, other.coefficient and self.growth.

2146cfe  Implemented BTerm._coerce_map_from_

9da7578  Item 21. BTerms are allowed to absorb exact terms.

11d56f4  Item 33, 53, 54 and 58.

676a401  Item 43. Removed valid_from lines from BTerm.can_absorb().

fec2942  Item 24. Fixed checking of variables when creating BTerms.

ed3b215  Item 8. Added BTerm.explanation() with an explanatory string for what valid_from really means

06128c1  Item 27. BTerm._repr_ added a test with multiple entries of values_from

comment:27 Changed 6 months ago by
I addressed items: 8, 21, 24, 27, 43, 47, 48, 49, 50, 51, 52, 53, 54 (33 ctd.), 55, 56, 57, 58
.
I have implemented a basic version of BTermMonoid._coerce_map_from_
to allow absorbtion of exact terms. This does not work yet. So items 16
and 21
are still open.
comment:28 followup: ↓ 29 Changed 6 months ago by
 Status changed from needs_review to needs_work
Thank you. Some remarks on previous items and a list of 5 items, most of which should be easily fixable.
 Concerning 21 and the coercion framework: I'd appreciate Benjamin's and/or Daniel's input on that.
 Concerning 8: I cannot remember having seen a method
.explanation
before, so I am unsure whether users will actually find it. I previously had thought of simply adding some text into a rather visible example (e.g. in the class docstring ofBTerm
). So I would appreciate other opinions on how to proceed. In the meantime, I keep 8 open in order to track this.
 Concerning 16: I think that this item simply disappeared in one of the iterations; in any case there is clearly no
NotImplementedError
inBTerm.can_absorb
anymore. And handling keys ofvalid_from
is now done inBTerm._absorb_
. So I consider 16 to be closed.
Thus as far as I am concerned, the following items are open from previous rounds:
 example for
valid_from
: It would be great to have an example where the parametervalid_from
is explained within this concrete situation.
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms.
New items:
BTerm._coerce_map_from_
: I think that the coefficient ring ofS
should also coerce into the coefficient ring ofself
(This is not mentioned in the Note box, but I think this is mentioned in some of the other term monoids.)
DecreasingGrowthElementError
: docstring: I think thatMonomialGrowthElement
should be replaced byGenericGrowthElement
(at some stage, we will also raise this error in other situations thanMonomialGrowthElement
and I assume that we will then not be aware of the fact that this docstring needs corrections.)
MonomialGrowthElement._find_minimum_
: typo "less then one" > "less than one"
MonomialGrowthElement._find_minimum_
: I think thatif not len(self.variable_names()):
could be stated as anassert self.variable_names(), f'{self.variable_names()} is empty'
because at this point, we have no idea how this should ever false, so an assertion seems to be more adequate. And it seems to be more pythonic to test for truthiness of the tupleself.variable_names()
instead of computing the length first.
BTerm.explanation
: Replace""""
by"""
.
comment:29 in reply to: ↑ 28 Changed 6 months ago by
Replying to cheuberg:
 Concerning 21 and the coercion framework: I'd appreciate Benjamin's and/or Daniel's input on that.
It is the conversion (not coercion) that is already failing. There seem to be two issues. First, there needs to be place during construction of an element, where it is said how to convert an exact term to an B
term. This was much easier with O
terms and done a bit implicitly (somewhere). Second (but related wrt to the code), there is a issue with passing on parameters (the valid_from
keyword). I will think about these issues and how to fix them, inparticular the second, tomorrow.
comment:30 followup: ↓ 36 Changed 6 months ago by
FWIW, I just had a look at the problem with conversion. There are two questions that need to be answered:
What should be the result of
sage: from sage.rings.asymptotic.growth_group import GrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = GrowthGroup('x^ZZ') sage: BT = TermMonoid('B', G, QQ) sage: ET = TermMonoid('exact', G, QQ) sage: t = ET(x^2, 5); t 5*x^2 sage: BT(t) # converting an exact term to a BTerm ???
My suggestion would be B(5*x^2, x>=0)
or something similar.
As for how this should be implemented: The conversion happens in GenericTermMonoid._element_constructor_
, and currently fails because when calling BT(t)
, the passed data
is an ExactTerm
, which thus branches into the elif
on line 1809 in which self._create_element_(data.growth, data.coefficient)
is returned  but as self
references BT
, a BTermMonoid
here, BTermMonoid._create_element_
is called which has valid_from
as a nonoptional keyword.
Now this is the point where we have several options how to fix this (this is more or less the second question), here are two possible ones:
 Make
valid_from
an optional keyword argument inBTermMonoid._create_element_
with default valueNone
, and add something along the lines ofif valid_from is None: valid_from = dict((var_name, self.coefficient_ring.zero()) for var_name in growth.variable_names())
to the method. However, note that this would also mean that something likeBT(x^2, 5)
wouldn't error out althoughvalid_from
was not specified; it would default to creating aBTerm
which is assumed to be valid for all x >= 0.
 Adapt
GenericTermMonoid._element_constructor_
and explicitly ask in the elifbranch starting on line 1809 whetherself
is an instance ofBTermMonoid
– and if so, construct the correspondingvalid_from
dictionary just like in the first case, then callself._create_element_
just like intended.
(a) brings an implicit assumption with it that could theoretically cause problems, but I think it is the more flexible and interfacefriendlier variant. I don't like checking for a concrete class in GenericTermMonoid._element_constructor_
as much because it *could* take away some of the flexibility the framework currently offers (one *could* want to implement a different BTerm
class; the term factory supports that in principle – but there are of course workarounds) as suggested in (b). A more flexible variant of (b) would be the implementation of BTermMonoid._element_constructor_
, but that also comes with some degree of code duplication / the need to refactor.
Let's discuss this further in our upcoming meeting.
comment:31 Changed 6 months ago by
 Commit changed from 06128c1aee93d70470bf47971cb8f68875ce6947 to 395fd73951c8f30522c0f4bef1ac60ddcf5af540
Branch pushed to git repo; I updated commit sha1. New commits:
4101e7b  Item 8 and 63. Removed BTerm.explanation. Moved the examples for valid_from into the docstring of BTerm.

7d02db8  Item 60. Replaced MonomialGrowthElement by GenericGrowthElement in DecreasingGrowthElementError.

bf39a80  Items 61 and 62. Fixed typo then > than. Changed calculating the length of the tuple, to test for truthiness of the tuple self.variables().

b75cb35  Item 59. BTerm._coerce_map_from_ the coefficient ring of S also coerces into the coefficient ring of self.

395fd73  Added check and doctests for logarithmic monomials in MonomialGrowthElement._find_minimum_()

comment:32 Changed 6 months ago by
 Status changed from needs_work to needs_review
I have addressed items 8, 59, 60, 61 and 62.
comment:33 Changed 6 months ago by
 Branch changed from u/ghthhagelmayer/initial_support_for_bterms to u/dkrenn/initial_support_for_bterms
comment:34 followups: ↓ 35 ↓ 39 ↓ 40 ↓ 50 Changed 6 months ago by
 Commit changed from 395fd73951c8f30522c0f4bef1ac60ddcf5af540 to 77717e4d71091ad09810b0fdc08133b84c17b046
 Reviewers set to Daniel Krenn
 Status changed from needs_review to needs_work
Thanks you for this code; it looks quite good already; I have a couple of minor comments:
 Use the same description of
valid_from
everywhere, e.g. the one ofBTerm
:+  ``valid_from``  dictionary mapping variable names to lower bounds + for the corresponding variable. The bound implied by this term is valid when + all variables are at least their corresponding lower bound
It is not used e.g. in.growth_group
 "Examples" at top of docstring of
BTerm
. Is there a particular reason for putting the examples at this unsusual position? If not, I suggest to move them to the EXAMPLESsection; then it can also be checked that they work properly.
 I suggest to put the
@experimental
decorator to the monoid.
 Consider putting the experimentalwarning in the docstring at the top of the file; maybe in the Varioussection. This
FutureWarning
will otherwise be somewhere in the middle due to #32229.
 In
__init__
, the codetry: coefficient = parent.coefficient_ring(coefficient) ...
seems to be unneccessary. This is done in the superclass. I think, the linesuper().__init__(parent=parent, growth=growth, coefficient=coefficient)
should be the first statement in this__init__
to deal with this issue.
Also the check for a zero coefficient can be removed as done in the superclass init.
Moreover, just seen that in
TermWithCoefficient.__init__
thesuper
should (for consistency) be moved to the top.
 The code
self.valid_from = valid_from
should only be done after everything is checked, not before.
 In
_absorb_
, the doctestsage: t1.absorb(t4)
fails because conversion is not implemented yet (#32229). I think, the wrong output should be removed and the test marked as by "# not tested, see #32229" (or similar) to avoid confusion.
 In
_absorb_
, the lineif self.growth < other.growth:
should rather beif not (self.growth >= other.growth):
right?
 In
_absorb_
, I think instead ofunion = {**self.valid_from, **other.valid_from} for variable_name in union:
the codefor variable_name in set().union(self.valid_from.keys(), other.valid_from.keys()):
would be clearer.
 In
_create_element_
, is the idea of the testsage: BT(x^3, 4, 10)
to make sure, that there is no positional third argument? If yes, this will be impossible anyways after #32215. If not, what is its purpose?
New commits:
3498449  Trac #31933 review: unify spelling Bterm

1933586  Trac #31933 review: simplfy doctests by using factory

77717e4  Trac #31933 review: minor changes to docstrings etc

comment:35 in reply to: ↑ 34 ; followup: ↓ 38 Changed 6 months ago by
Replying to dkrenn:
Thanks you for this code; it looks quite good already; I have a couple of minor comments:
New commits:
3498449 Trac #31933 review: unify spelling Bterm
1933586 Trac #31933 review: simplfy doctests by using factory
77717e4 Trac #31933 review: minor changes to docstrings etc
I've added a couple of very minor reviewer commits (seemed easier than writing these up; please have a look).
comment:36 in reply to: ↑ 30 Changed 6 months ago by
comment:37 Changed 6 months ago by
I did not check what from <= 63 is still open and what not.
comment:38 in reply to: ↑ 35 Changed 6 months ago by
Replying to dkrenn:
Thank you for looking at the code and for the minor commits. It looks good to me. I will work on the points you mentioned.
Replying to dkrenn:
Thanks you for this code; it looks quite good already; I have a couple of minor comments:
New commits:
3498449 Trac #31933 review: unify spelling Bterm
1933586 Trac #31933 review: simplfy doctests by using factory
77717e4 Trac #31933 review: minor changes to docstrings etc
I've added a couple of very minor reviewer commits (seemed easier than writing these up; please have a look).
comment:39 in reply to: ↑ 34 ; followup: ↓ 42 Changed 6 months ago by
comment:40 in reply to: ↑ 34 ; followup: ↓ 43 Changed 6 months ago by
Replying to dkrenn:
 "Examples" at top of docstring of
BTerm
. Is there a particular reason for putting the examples at this unsusual position? If not, I suggest to move them to the EXAMPLESsection; then it can also be checked that they work properly.
I have put this Examples section at the top for better visibility. The main purpose of this block is to explain what valid_from
actually means for a Bterm.
comment:41 Changed 6 months ago by
 Dependencies set to #32153
comment:42 in reply to: ↑ 39 Changed 6 months ago by
Replying to ghthhagelmayer:
Replying to dkrenn:
 In
_create_element_
, is the idea of the testsage: BT(x^3, 4, 10)to make sure, that there is no positional third argument? If yes, this will be impossible anyways after #32215. If not, what is its purpose?Yes, that is the idea. Should the test be removed then?
Probably the best, since it would be removed later anyways.
comment:43 in reply to: ↑ 40 ; followup: ↓ 45 Changed 6 months ago by
Replying to ghthhagelmayer:
Replying to dkrenn:
 "Examples" at top of docstring of
BTerm
. Is there a particular reason for putting the examples at this unsusual position? If not, I suggest to move them to the EXAMPLESsection; then it can also be checked that they work properly.I have put this Examples section at the top for better visibility. The main purpose of this block is to explain what
valid_from
actually means for a Bterm.
I see. This seems not quite easy to do, however. If there is code, it should be tested. We can make run the test directly there; this would give something like
We first explain what Bterms are. With :: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) Bterms are, for example, :: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3 a term whose absolute value is bounded by `5x^2` for `x >= 3`, :: sage: T(x^3, coefficient=42, valid_from={'x': 15, 'y': 15}) # not tested a term whose absolute value is bounded by `42x^3` for `x >= 15` and ` y >= 15`, or :: sage: T(x^3*y^2, coefficient=42, valid_from={'x': 10, 'y': 20}) # not tested a term whose absolute value is bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`.
We need the not tested
as two variables do not work right now (as I understand).
As an alternative, we could do something like
A Bterm represents all functions which (in absolute value) is bounded by the given ``growth`` and ``coefficient`` for the parameters given by ``valid_from``. For example, we have terms that represent functions  bounded by `5x^2` for `x >= 3` (see below for the actual example),  bounded by `42x^3` for `x >= 15` and ` y >= 15`, or  bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`. INPUT: ... EXAMPLES:: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) We revisit the example from the introduction:: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3
What do you think?
comment:44 Changed 6 months ago by
 Branch changed from u/dkrenn/initial_support_for_bterms to u/ghthhagelmayer/initial_support_for_bterms
comment:45 in reply to: ↑ 43 ; followup: ↓ 52 Changed 6 months ago by
 Commit changed from 77717e4d71091ad09810b0fdc08133b84c17b046 to 50a06cb4e33f149cc6b187f168d6322913b16fdd
Replying to dkrenn:
Replying to ghthhagelmayer:
Replying to dkrenn:
 "Examples" at top of docstring of
BTerm
. Is there a particular reason for putting the examples at this unsusual position? If not, I suggest to move them to the EXAMPLESsection; then it can also be checked that they work properly.I have put this Examples section at the top for better visibility. The main purpose of this block is to explain what
valid_from
actually means for a Bterm.I see. This seems not quite easy to do, however. If there is code, it should be tested. We can make run the test directly there; this would give something like
We first explain what Bterms are. With :: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) Bterms are, for example, :: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3 a term whose absolute value is bounded by `5x^2` for `x >= 3`, :: sage: T(x^3, coefficient=42, valid_from={'x': 15, 'y': 15}) # not tested a term whose absolute value is bounded by `42x^3` for `x >= 15` and ` y >= 15`, or :: sage: T(x^3*y^2, coefficient=42, valid_from={'x': 10, 'y': 20}) # not tested a term whose absolute value is bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`.We need the
not tested
as two variables do not work right now (as I understand).As an alternative, we could do something like
A Bterm represents all functions which (in absolute value) is bounded by the given ``growth`` and ``coefficient`` for the parameters given by ``valid_from``. For example, we have terms that represent functions  bounded by `5x^2` for `x >= 3` (see below for the actual example),  bounded by `42x^3` for `x >= 15` and ` y >= 15`, or  bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`. INPUT: ... EXAMPLES:: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) We revisit the example from the introduction:: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3What do you think?
I like the first approach. I will implement it.
We need the
not tested
as two variables do not work right now (as I understand).
Two variables do not work right now. The representation works for two variables though.
Last 10 new commits:
b8868eb  Item 70. Remove wrong output and mark the doctest with # not tested.

c2d9603  Items 71 and 72. Small corrections in BTerm._absorb_ for better readability.

091271d  Item 67. Moved WARNINGBlock to the Various section for better visibility.

af7ca87  Refactor ExactTerm._repr_ to TermWithCoefficient._product_repr_

f3b1c46  Revert TermWithCoefficient._repr_ to be more descriptive.

7fb17ce  rename _product_repr_ to _repr_product_

7af743f  added docstring to TermWithCoefficient._repr_product_

1db8a42  Changed the call to the product representation in ExactTerm._repr_ to self._repr_product_(latex=latex)

c1e50c4  Merge remotetracking branch 'trac/u/ghthhagelmayer/refactor__repr__of_some_terms' into t/31933/initial_support_for_bterms

50a06cb  Trac #31933 Item 67. Fix WARNINGBlock.

comment:46 followup: ↓ 51 Changed 6 months ago by
 Commit changed from 50a06cb4e33f149cc6b187f168d6322913b16fdd to 8e5e0e4ce291ccf71b5a78748541725f75e92fd7
Branch pushed to git repo; I updated commit sha1. New commits:
230bfe7  Trac #31933. Changed BTerm._repr_ to use self._repr_product and make the output shorter and fix doctests.

b28061e  Trac #31933. Addded latex representation to BTerm._repr_

eec9024  Trac #31933. Item 66. Move experimental tag to BTermMonoid.

8082d47  Trac #31933. Item 73. Remove doctest which will be impossible after #32215.

8e5e0e4  Trac #31933. Item 65. Modify examples with explanation of valid_from to test the examples.

comment:47 Changed 6 months ago by
 Status changed from needs_work to needs_review
I worked on items 6473
and merged #32153 into this ticket.
A review would be appreciated.
comment:48 Changed 6 months ago by
 Commit changed from 8e5e0e4ce291ccf71b5a78748541725f75e92fd7 to 76d9bcc05abf72d45fe383e5eac89c3142a87c91
Branch pushed to git repo; I updated commit sha1. New commits:
76d9bcc  Trac #31933. Fix docbuild issue

comment:49 Changed 6 months ago by
It seems that there are missing the reviewer commit done on u/dkrenn/initial_support_for_bterms
:
* 77717e4d71  Trac #31933 review: minor changes to docstrings etc (9 days ago) [Daniel Krenn] * 1933586595  Trac #31933 review: simplfy doctests by using factory (9 days ago) [Daniel Krenn] * 3498449102  Trac #31933 review: unify spelling Bterm (9 days ago) [Daniel Krenn]
comment:50 in reply to: ↑ 34 Changed 6 months ago by
Replying to dkrenn:
 Consider putting the experimentalwarning in the docstring at the top of the file; maybe in the Varioussection. This
FutureWarning
will otherwise be somewhere in the middle due to #32229.
091271d Item 67. Moved WARNINGBlock to the Various section for better visibility.
Various
and =======
need to be next to each other, because the =
mark a heading.
With my suggestion I meant to also put the actual experimentalwarning at the top, so something like:
Various ======= .. WARNING:: The code for :class:`BTerms <BTermMonoid>` is experimental, so a warning is thrown when a :class:`BTerm` is created for the first time in a session (see :class:`sage.misc.superseded.experimental`). :: sage: from sage.rings.asymptotic.growth_group import GrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: T = TermMonoid('B', growth_group=GrowthGroup('x^ZZ'), coefficient_ring=QQ) doctest:warning ... 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.
(not tested, docs not built)
comment:51 in reply to: ↑ 46 Changed 6 months ago by
comment:52 in reply to: ↑ 45 Changed 6 months ago by
Replying to ghthhagelmayer:
I see. This seems not quite easy to do, however. If there is code, it should be tested. We can make run the test directly there; this would give something like
We first explain what Bterms are. With :: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) Bterms are, for example, :: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3 a term whose absolute value is bounded by `5x^2` for `x >= 3`, :: sage: T(x^3, coefficient=42, valid_from={'x': 15, 'y': 15}) # not tested a term whose absolute value is bounded by `42x^3` for `x >= 15` and ` y >= 15`, or :: sage: T(x^3*y^2, coefficient=42, valid_from={'x': 10, 'y': 20}) # not tested a term whose absolute value is bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`.We need the
not tested
as two variables do not work right now (as I understand).As an alternative, we could do something like
A Bterm represents all functions which (in absolute value) is bounded by the given ``growth`` and ``coefficient`` for the parameters given by ``valid_from``. For example, we have terms that represent functions  bounded by `5x^2` for `x >= 3` (see below for the actual example),  bounded by `42x^3` for `x >= 15` and ` y >= 15`, or  bounded by `42 x^3 y^2` for `x >= 10` and y >= 20`. INPUT: ... EXAMPLES:: sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid sage: G = MonomialGrowthGroup(ZZ, 'x') sage: T = TermMonoid('B', growth_group=G, coefficient_ring=QQ) We revisit the example from the introduction:: sage: T(x^2, coefficient=5, valid_from={'x': 3}) BTerm with coefficient 5, growth x^2 and valid for x >= 3What do you think?
I like the first approach. I will implement it.
I know that I suggested both approached; I, however, have second thoughts about the first apporach, since it clearly violates the style guidelines of the developer guide, as there is SageMathcode before the input block and the input block is quite late. Maybe we should discuss this once more.
comment:53 Changed 6 months ago by
 Commit changed from 76d9bcc05abf72d45fe383e5eac89c3142a87c91 to 5a3c5b1d928fdf1f32d9f57f77bf866e814b9ae6
Branch pushed to git repo; I updated commit sha1. New commits:
7e758a3  Trac #31933 review: unify spelling Bterm

786bd48  Trac #31933 review: simplfy doctests by using factory

760a9df  Trac #31933 review: minor changes to docstrings etc

0afd7bf  Trac #31933 Fix doctests after merge

33452c3  Trac #31933. Item 67. Fix WARNINGBlock and Various formatting.

bf9ff54  Trac #31933. Avoid trailing comma in latex repr.

5a3c5b1  Trac #31933. Item 65. Fix Examples for valid_from with explanation.

comment:54 Changed 6 months ago by
Thank you for your comments. I merged the missing reviewer commits and corrected the small issues.
New commits:
7e758a3  Trac #31933 review: unify spelling Bterm

786bd48  Trac #31933 review: simplfy doctests by using factory

760a9df  Trac #31933 review: minor changes to docstrings etc

0afd7bf  Trac #31933 Fix doctests after merge

33452c3  Trac #31933. Item 67. Fix WARNINGBlock and Various formatting.

bf9ff54  Trac #31933. Avoid trailing comma in latex repr.

5a3c5b1  Trac #31933. Item 65. Fix Examples for valid_from with explanation.

comment:55 Changed 6 months ago by
 Status changed from needs_review to needs_work
Thank you for the changes. I had a look on the commits since my last comments; we are all aware that conversion is postponed to #32229, but I keep
BTerm.can_absorb
: I think thatBTerms
should be able to absorb exact terms
on the list, just for the record.
New items:
 (f'up to 8, 65)
BTerm
docstring: "all functions which (in absolute value) is bounded" > "all functions which (in absolute value) are bounded".
MonomialGrowthElement._find_minimum_
: I suggest to rewrite the error message introduced in 395fd73951c8f to say "Minimum of ... is not implemented" (add "Minimum of") such that this is more understandable.
 (f'up to 64)
sitepackages/sage/rings/asymptotic/growth_group.py:docstring of sage.rings.asymptotic.growth_group.GenericGrowthElement._find_minimum_:8: WARNING: Bullet list ends without a blank line; unexpected unindent
; furthermore, I do not think that the docstring fits forfind_minimum
("The bound implied by this term is valid" is good forBTerm
s, but not for_find_minimum_
). I suggest to revert the changes made in 1c1ebd41 ingrowth_group.py
only.
BTerm
docstring: "(see below for the actual example)," should be moved to a position where it does not only concern the first example, but all three examples.
BTerm
docstring: Missing empty line after "We revisit the example from the introduction::" leads to destroyed formatting of the examples
 In the various section: remove "Lterms" from the todo box of things that need to be implemented, add your name to authors and acknowledgement.
BTerm._latex_
: Replace>=
by\ge
.
BTerm
docstring: The old exampleBT_QQ(x, 3, valid_from={'x': 20})
plus its associated definitions can now be removed because we now have the new examples which are explained in more detail.
BTerm
docstring: I suggest to break the examples block into three blocks and copy the short explanation from the start of the docstring after each of the examples (this is some redundancy, but avoids the need for scrolling, and might make things clearer; in some ways, it combines the advantages of both approaches discussed for the examples).
comment:56 followups: ↓ 61 ↓ 62 Changed 6 months ago by
 Commit changed from 5a3c5b1d928fdf1f32d9f57f77bf866e814b9ae6 to c8c8ec486fad105cc65e68b1889ff95575b49c97
Branch pushed to git repo; I updated commit sha1. New commits:
bf880c7  Trac #31933. Item 74 fix BTerm docstring.

447f64f  Trac #31933. Item 75. Add more meaningful Error message

2a86c51  Trac #31933. Item 81: remove an example

2194881  Trac #31933. Item 78: add missing empty line

b8e7b86  Trac #31933. Item 82: Split examples block into three parts to have clearer explanations without scrolling.

e494cb7  Trac #31933. Item 80: BTerm._latex_: replace >= by \ge.

04c07e4  Trac #31933. Item 79: Add Author and acknowledgment. Remove Lterms from todo box.

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.

comment:57 Changed 6 months ago by
Thank you. I still have a few rather trivial comments.
 (f'up to 81):
BTerm.__init__
docstring:MonomialGrowthGroup
is imported, but unused.
BTerm.__init__
: I suggest to replace "We revisit the example from the introduction:" by "We revisit the examples from the introduction:" because it is more than one example
BTerm.__init__
: Replace>=
by\ge
in all TeX formatted code
 (f'up to 80)
BTerm._latex_
: I think thatf'{variable} \ge {value}'
should befr'{variable} \ge {value}'
orf'{variable} \\ge {value}'
to avoid the warning4162: DeprecationWarning: invalid escape sequence \g
 (f'up to 76):
MonomialGrowthElement._find_minimum_
: please use the same description for thevalid_from
as inGenericGrowthElement._find_minimum_
.
 (f'up to 79): I think that in the acknowledgements, there should be consistency between "supported by the Google Summer of Code" and "supported by Google Summer of Code".
comment:58 Changed 6 months ago by
 Commit changed from c8c8ec486fad105cc65e68b1889ff95575b49c97 to 2e46c20aee507515cdb19f3847d0a337f840b535
Branch pushed to git repo; I updated commit sha1. New commits:
d076a0d  Trac #31933. Item 83: remove unused import

343ddd8  Trac #31933. Item 84: correct spelling

ff4747b  Trac #31933. Item 86: make fstring an frstring

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

comment:59 Changed 6 months ago by
 Status changed from needs_work to needs_review
I have worked on items 83, 84, 85, 86, 87, 88
.
comment:60 Changed 6 months ago by
 Milestone changed from sage9.4 to sage9.5
 Reviewers changed from Daniel Krenn to Daniel Krenn, Clemens Heuberger
Thank you. I have no further comments at this point.
Let us keep this ticket open for some more days to give Daniel a chance to have a look at it in its present form (in particular the examples in the docstring of BTerm.__init__
); the overall release process is now in rc stage, so tickets like this have no chance of getting merged before the first beta releases for 9.5.
comment:61 in reply to: ↑ 56 Changed 6 months ago by
Replying to git:
c8c8ec4 Trac #31933. Item 76: revert GenericGrowthElement._find_minimum_ Input explanation introduced in 1c1ebd41. Add missing empty line.
 Is there a reason for not adapting the docstring description of the parameter
valid_from
in the term monoid file? If not, then this should also have the same description
comment:62 in reply to: ↑ 56 Changed 6 months ago by
Replying to git:
b8e7b86 Trac #31933. Item 82: Split examples block into three parts to have clearer explanations without scrolling.
 I suggest to replace
+ This is a term bounded by `5x^2` for `x >= 3`. + ::
by
+ This is a term bounded by `5x^2` for `x >= 3`::
to have a colon preceding the code to make the reference of the "This" clearer. (3 occurrences)
comment:63 Changed 6 months ago by
 Status changed from needs_review to needs_work
Except for the two minor (almost trivial) comments 89 and 90, this ticket is fine for me.
comment:64 Changed 6 months ago by
Just had a look at the code because of #32214 and found two more very small things:
 I think, we should remove "big" in the only occurrence
+ Parent for asymptotic big `B`terms.
to stay consistent.
 For consistency, we should replace the two
`B`term
by Bterm
.
comment:65 Changed 6 months ago by
 Commit changed from 2e46c20aee507515cdb19f3847d0a337f840b535 to f631489e308d859ee22ff2464cc28724f1789782
Branch pushed to git repo; I updated commit sha1. New commits:
f631489  Trac #31933. Items: 90, 91, 92: minor corrections in the documentation

comment:66 Changed 5 months ago by
 Keywords gsoc2021 added; gsoc21 removed
comment:67 Changed 5 months ago by
 Status changed from needs_work to needs_review
comment:68 Changed 5 months ago by
 Reviewers changed from Daniel Krenn, Clemens Heuberger to Daniel Krenn, Clemens Heuberger, Benjamin Hackl
 Status changed from needs_review to positive_review
I've reviewed the changes from when I last looked at this ticket up to now and have no further comments.
89 seems to be unresolved, but I think it is fine that MonomialGrowthElement._find_minimum_
has a different description for valid_from
than the TermMonoid
– semantically, the focus is different.
Thanks, everyone!
comment:69 Changed 5 months ago by
 Branch changed from u/ghthhagelmayer/initial_support_for_bterms to f631489e308d859ee22ff2464cc28724f1789782
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
add Docstring to BTerm __init__
changed strings in class BTerm to fstrings
Add representation to classes BTerm and BTermMonoid. Change super(BTerm, self) to super() in __init__
added descriptions
make represenation more user friendly
refactor can_absorb and _absorb_ functions
fix BTermMonoid representation string
add description to can_absorb
fix can_absorb() function
fix absorb function and doctest