Opened 3 months ago

Closed 3 weeks ago

#31933 closed enhancement (fixed)

Initial support for BTerms

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

Status badges

Description (last modified by gh-thhagelmayer)

Adding initial support for BTerms implementing the absorb() and can_absorb() functions.

Change History (69)

comment:1 Changed 3 months ago by gh-thhagelmayer

  • Authors set to Thomas Hagelmayer
  • Cc cheuberg behackl added
  • Component changed from PLEASE CHANGE to asymptotic expansions
  • Description modified (diff)
  • Keywords gsoc21 asymptotics added
  • Owner changed from (none) to gh-thhagelmayer
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 3 months ago by gh-thhagelmayer

  • Branch set to u/gh-thhagelmayer/initial_support_for_bterms

comment:3 Changed 3 months ago by git

  • Commit set to 463bcba2278887d598218a4660b62d38989e63a8

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

5715b30add Docstring to BTerm __init__
2d77f2fchanged strings in class BTerm to f-strings
16d6095Add representation to classes BTerm and BTermMonoid. Change super(BTerm, self) to super() in __init__
c73b3bcadded descriptions
b88624fmake represenation more user friendly
97d3a1crefactor can_absorb and _absorb_ functions
b88178afix BTermMonoid representation string
9c2e69dadd description to can_absorb
be4ff66fix can_absorb() function
463bcbafix absorb function and doctest

comment:4 Changed 3 months ago by git

  • Commit changed from 463bcba2278887d598218a4660b62d38989e63a8 to 8b40bf9b1513c36ed7b18f249164232ec213fa1e

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

8b40bf9fix typo in class BTerm doctest

comment:5 Changed 3 months ago by git

  • Commit changed from 8b40bf9b1513c36ed7b18f249164232ec213fa1e to 0f0b4d93a2f4b46ae720a40e79be61d57639d5b3

Branch pushed to git repo; I updated commit sha1. This was a forced push. Last 10 new commits:

0e89713changed strings in class BTerm to f-strings
809ce70Add representation to classes BTerm and BTermMonoid. Change super(BTerm, self) to super() in __init__
f78de5eadded descriptions
378165dmake represenation more user friendly
a7739e9refactor can_absorb and _absorb_ functions
99851f2fix BTermMonoid representation string
0a8d14eadd description to can_absorb
d4c2609fix can_absorb() function
aeae824fix absorb function and doctest
0f0b4d9fix typo in class BTerm doctest

comment:6 follow-up: Changed 3 months ago by dkrenn

Just some comments that got to me when scrolling through the current code (well being aware that it is work in progress):

  1. Consider passing **kwds instead of valid_form in
    +    def _element_constructor_(self, data, coefficient=None, valid_from=None):
    
  1. 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)

  1. Remove empty line in _create_element_ before the return.
  1. 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 3 months ago by gh-thhagelmayer

Replying to dkrenn:

Thank you for your suggestions!

  1. 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 3 months ago by cheuberg

Just some comments from my side at this point:

  1. BTerm.__init__: Please add tests for all possible exceptions which may be raised in this method
  1. ./sage --docbuild --include-tests-blocks --underscore reference/asymptotic html gave a warning: local/lib/python3.9/site-packages/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 the EXAMPLES block.
  1. TermMonoidFactory.create_key_and_extra_args: I would prefer to write "has to be either 'exact', 'O', 'B' or an instance" in the ValueError (commas instead of repeated "or").
  1. example for valid_from
  1. BTerm.__init__: description of the parameter valid_from: "vriable" -> "variable", "the bound by this term" -> "The bound implied by this term".
  1. 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#string-literal-concatenation
  1. 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_)
  1. BTerm._repr_, last example: I think that BT_QQ(x, 3, {'x': 20, 'm': 5}) should raise an error because m is not a valid variable understood by the growth group G.
  1. BTerm.can_absorb, docstring: "An BTerm can absorb" -> "A BTerm can absorb"
  1. BTerm.can_absorb, docstring: "with weaker or equal growth", change the code accordingly, and add examples for absorption of BTerms of different growths.
  1. BTerm.can_absorb: I think that we should discuss whether t2.can_absorb(t1) should be True or False; more specifically, whether it is allowable to change valid_from during absorption. It should also be discussed whether we want to use different parents for different values in valid_from.
  1. BTerm.can_absorb: The NotImplementedError seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys of valid_from must agree, which should be checked somewhere).
  1. BTerm._absorb_: I think that neither the example nor the code are correct. Dividing by self.valid_from[str(self.growth)] is only valid for situations where the exponents of x 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).
  1. BTerm._create_element_: Empty INPUT, TESTS sections, missing OUTPUT section.
  1. TermMonoidFactory docstring: replace . by , after ExactTermMonoid.
  1. TermMonoidFactory docstring: I think that "(capital letter B)" can be removed, there is no danger of confusion here (in contrast to O-Terms where the capital letter O might be confused with the digit 0).
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.

comment:9 Changed 3 months ago by git

  • Commit changed from 0f0b4d93a2f4b46ae720a40e79be61d57639d5b3 to 4060b6c083ddf6806cedf682d746457687321f34

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

7be8d49Move binary operator back down after accidentally moving it up
1f698d3Passing **kwds in _element_constructor_ instead of valid_from. Adjusting code to be compatible with changes
5d9337bfix indentation of EXAMPLES block in absorb()
0eeb50dAdded check in init. All defined variables in valid_from have to occur in the term.
3ec4a6aImplemented find_minimum() in growth_group.py.
ee337e2added tests to init from BTerm
8e754d7replacing '\' and '+ \' with parenthesis' in BTerm._repr_ and BTermMonoid._repr_
9c7771ereplaced 'or' with ',' in ValueError text in create_key_and_extra_args
b5f1470Small corrections in dosctrings. Correcting typos and removing trailing dots.
4060b6cRemove unused example from BTerm.can_absrob()

comment:10 Changed 3 months ago by cheuberg

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:

  1. Missing empty line after WARNING:: leads to non-proper formatting of the warning.
  1. The link to sage.misc.superseded.experimental is not formatted properly.
  1. Follow-up 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 of valid_from are enforced to at most contain the variables of the actual growth element.
  1. BTerm._absorb_ should not inspect self.growth.exponent or other.growth.exponent: This only works for MonomialGrowthElement. Instead, self.growth should be compared to other.growth (comparison of growth elements is implemented in the growth group which will know what to do).
  1. ._element_constructor_: There is now an try/except/else construct where the else 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).
  1. BTerm._repr_ is now untested (since 0eeb50da71) in the case of several entries of values_from
  1. BTerm._absorb_: Computation of valid_from_new will always yield a one-key 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.
  1. can_absorb: Missing empty line after TESTS:: leads to unproper formatting of tests (when using ./sage --docbuild --include-tests-blocks)
  1. can_absorb: I think that absorption should only happen if self.growth >= other.growth. In particular, B(x^3) can absorb B(x^2) (because it is larger), but B(x^2) should not absorb B(x^3).
  1. GenericGrowthElement._find_minimum_: please add docstring.
  1. GenericGrowthElement._find_minimum_ and MonomialGrowthElement._find_minimum_: I think that it would be simpler not to have other in the signature of this method. If B(g) absorbs B(h) for some g >= h, then absorb could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then call q._find_minimum_.
  1. 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)
  1. MonomialGrowthElement._find_minimum_: I think that the signature should be the same as for GenericGrowthElement._find_minimum_: the last parameter should be valid_from instead of valid_from_bound. I think that it should be the growth element's task to extract the correct variable.
  1. MonomialGrowthElement._find_minimum_: I suggest the following one-sentence description: "Find the minimum of this growth element over the range implied by valid_from"
  1. BTerm._absorb_: I think that t2 should not absorb t1 (related to 30 and 33).

comment:11 Changed 3 months ago by behackl

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 BTerms 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 BTerms with AsymptoticRing, but that also means I wouldn't put too much effort into polishing doctests using the current representation.

Some further minor comments:

  1. 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 with
    sage: from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid
    
  1. The one-sentence description for MonomialGrowthElement._find_minimum_ suggested by Clemens above also works for GenericGrowthElement._find_minimum_.
  1. BTerm.can_absorb: In the NOTE block of the docstring, the second occurrence of BTerm should also be a reference. Also, make sure to add a newline after TESTS::.

comment:12 follow-up: Changed 3 months ago by dkrenn

  1. 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, non-indented ending (the return above) of an indented block.

comment:13 in reply to: ↑ 12 Changed 3 months ago by behackl

Replying to dkrenn:

  1. 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, non-indented ending (the return above) of an indented block.

I think this is 26 (but with more explanations). :-)

comment:14 Changed 3 months ago by git

  • Commit changed from 4060b6c083ddf6806cedf682d746457687321f34 to 9fffc5a1de1b0b89208b34de3866b5034ad444a3

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

29cc561Item 26 and 40. Removing else of the try-block and returning as before
3e1f74fItems 22, 29, 39. Added missing empty lines after WARNING and TEST. BTerm.can_absorb() NOTES block, fixed BTerm formatting.
9fffc5aItem 19 and 20. Fix TermMonoidFactory docstring.

comment:15 Changed 3 months ago by git

  • Commit changed from 9fffc5a1de1b0b89208b34de3866b5034ad444a3 to 5961ac9b4e36f08674c6bc6db8091dfd2430f2eb

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

5961ac9added example to BTerm._absorb_ where absorbtion fails.

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

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 3 months ago by gh-thhagelmayer

  • Status changed from new to needs_review

comment:18 in reply to: ↑ 16 Changed 3 months ago by cheuberg

  • Status changed from needs_review to needs_work

Replying to gh-thhagelmayer:

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: https://seafile.aau.at/f/7f466538ce624d15b842/ In other words, it is not a link.

As far as I am aware, the following items are open from previous rounds:

  1. example for valid_from
  1. BTerm.can_absorb, docstring: "with weaker or equal growth" (rest of the former 14 is done)
  1. BTerm.can_absorb: The NotImplementedError seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys of valid_from must agree, which should be checked somewhere).
  1. BTerm._create_element_: Empty INPUT, TESTS sections, missing OUTPUT section.
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.
  1. The link to sage.misc.superseded.experimental is not formatted properly.
  1. Follow-up 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 of valid_from are enforced to at most contain the variables of the actual growth element.
  1. BTerm._absorb_ should not inspect self.growth.exponent or other.growth.exponent: This only works for MonomialGrowthElement. Instead, self.growth should be compared to other.growth (comparison of growth elements is implemented in the growth group which will know what to do).
  1. BTerm._repr_ is now untested (since 0eeb50da71) in the case of several entries of values_from
  1. BTerm._absorb_: Computation of valid_from_new will always yield a one-key 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.
  1. GenericGrowthElement._find_minimum_: please add docstring.
  1. GenericGrowthElement._find_minimum_ and MonomialGrowthElement._find_minimum_: I think that it would be simpler not to have other in the signature of this method. If B(g) absorbs B(h) for some g >= h, then absorb could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then call q._find_minimum_.
  1. 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)
  1. MonomialGrowthElement._find_minimum_: I think that the signature should be the same as for GenericGrowthElement._find_minimum_: the last parameter should be valid_from instead of valid_from_bound. I think that it should be the growth element's task to extract the correct variable.
  1. MonomialGrowthElement._find_minimum_: I suggest the following one-sentence description: "Find the minimum of this growth element over the range implied by valid_from"
  1. The one-sentence description for MonomialGrowthElement._find_minimum_ suggested by Clemens above also works for GenericGrowthElement._find_minimum_.

A few new items (some of them not related to the changes, but new findings in previously pushed code, sorry):

  1. BTerm.can_absorb, docstring, one-sentence description: replace `BTerm` by ``BTerm``
  1. BTerm.can_absorb: The code
    if self.growth >= other.growth:
        return True
    else:
        return False
    

could be simplified to return self.growth >= other.growth.

  1. BTerm.can_absorb: I fear I forgot to mention the following in the last round: I do not think that the restriction if key in other.valid_from.keys() is appropriate: we intend to fix valid_from during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
  1. BTerm.__init__: missing empty line after TESTS::.

comment:19 follow-up: Changed 2 months ago by git

  • Commit changed from 5961ac9b4e36f08674c6bc6db8091dfd2430f2eb to 41b1974d36432460fc45a63d390c7e088fee46cc

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

218d166Added example for multivariate BTerm in BTerm._init_
5dad24aItem 18 BTerm._create_element_ Filled out INPUT and TESTS section, added OUTPUT section.
d4b5f99Item 23. Formatted warning properly so that sage.misc.superseded.experimental is a link.
365971dItem 44. BTerm._init_ addd empty line after TESTS.
6a8d7cfItem 41. Fix docstring.
41b1974Item 14. Fix BTerm.can_absorb docstring.

comment:20 in reply to: ↑ 19 Changed 2 months ago by behackl

  1. The test you have added in 218d166 should assign x and y as the variables of the growth group B, like
    sage: x, y = B('x'), B('y')
    

as the NameError currently thrown has nothing to do with the implementation.

  1. (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, namely valid_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 follow-up). 23 also looks good to me, but I didn't look at the built documentation.

comment:21 Changed 2 months ago by cheuberg

23: This does not yet render properly:

https://seafile.aau.at/f/64b6704350b540d9838d/

As far as I am aware, the following items are open from previous rounds:

  1. example for valid_from: It would be great to have an example where the parametervalid_from is explained within this concrete situation.
  1. BTerm.can_absorb: The NotImplementedError seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys of valid_from must agree, which should be checked somewhere).
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.
  1. The link to sage.misc.superseded.experimental is not formatted properly.
  1. Follow-up 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 of valid_from are enforced to at most contain the variables of the actual growth element.
  1. BTerm._absorb_ should not inspect self.growth.exponent or other.growth.exponent: This only works for MonomialGrowthElement. Instead, self.growth should be compared to other.growth (comparison of growth elements is implemented in the growth group which will know what to do).
  1. BTerm._repr_ is now untested (since 0eeb50da71) in the case of several entries of values_from
  1. BTerm._absorb_: Computation of valid_from_new will always yield a one-key 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.
  1. GenericGrowthElement._find_minimum_: please add docstring.
  1. GenericGrowthElement._find_minimum_ and MonomialGrowthElement._find_minimum_: I think that it would be simpler not to have other in the signature of this method. If B(g) absorbs B(h) for some g >= h, then absorb could first compute q = g/h (which is something the growth elements should be able to compute out of the box) and then call q._find_minimum_.
  1. 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)
  1. MonomialGrowthElement._find_minimum_: I think that the signature should be the same as for GenericGrowthElement._find_minimum_: the last parameter should be valid_from instead of valid_from_bound. I think that it should be the growth element's task to extract the correct variable.
  1. MonomialGrowthElement._find_minimum_: I suggest the following one-sentence description: "Find the minimum of this growth element over the range implied by valid_from"
  1. The one-sentence description for MonomialGrowthElement._find_minimum_ suggested by Clemens above also works for GenericGrowthElement._find_minimum_.
  1. BTerm.can_absorb: The code
    if self.growth >= other.growth:
        return True
    else:
        return False
    

could be simplified to return self.growth >= other.growth.

  1. BTerm.can_absorb: I fear I forgot to mention the following in the last round: I do not think that the restriction if key in other.valid_from.keys() is appropriate: we intend to fix valid_from during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
  1. The test you have added in 218d166 should assign x and y as the variables of the growth group B, like
    sage: x, y = B('x'), B('y')
    

as the NameError currently thrown has nothing to do with the implementation.

  1. (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, namely valid_from. The general form, **kwds is mainly important for the generic _element_constructor_; after that we know which arguments we can expect.

New items:

  1. BTermMonoid description of input variable coefficient_ring: Remove trailing full stop.
  1. BTermMonoid._create_element_: Replace TESTS: by TESTS::.

comment:22 Changed 2 months ago by git

  • Commit changed from 41b1974d36432460fc45a63d390c7e088fee46cc to 78c408f9b6f7215e7a38aad23bcbd6cfd925f097

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

5a3429bItems 25, 28, 31, 32, 34, 35, 38. Rewriting BTerm._absorb_, MonomialGrowthElement._find_minimum_ and GenericGrowthElement._find_minimum_
cecb744Item 45. Correction of the multivariate doctest
e6c19f2Items 24 and 43. BTerm.can_absorb: be more lenient on variable checking
6e64780Item 18 cont. BTermMonoid._create_element_: rename **kwds to valid_from
78c408fItem 27. BTerm._repr_: Add doctest for the case of multiple valid_from values

comment:23 Changed 2 months ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review

With the latest commit I consider following items to be open:

  1. example for valid_from
  1. BTerm.can_absorb: The NotImplementedError seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys of valid_from must agree, which should be checked somewhere).
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.
  1. 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 2 months ago by cheuberg

  • 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 contains x and y),

sage: BT_ZZ(x^3*y^2, 42, valid_from={'x': 10})

should be invalid because the term contains x and y, but y is missing in valid_from and finally

sage: 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 with valid_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:

  1. example for valid_from: It would be great to have an example where the parametervalid_from is explained within this concrete situation.
  1. BTerm.can_absorb: The NotImplementedError seems to be unnecessary because the following code should mostly work for arbitrary number of variables (but the keys of valid_from must agree, which should be checked somewhere).
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.
  1. The link to sage.misc.superseded.experimental is not formatted properly.
  1. Follow-up 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 of valid_from are enforced to at most contain the variables of the actual growth element.
  1. BTerm._repr_ is now untested (since 0eeb50da71) in the case of several entries of values_from
  1. BTerm.can_absorb: I fear I forgot to mention the following in the last round: I do not think that the restriction if key in other.valid_from.keys() is appropriate: we intend to fix valid_from during absorption; as long as all BTerms involved are well defined (according to 24) there should not be any problem.
  1. BTermMonoid description of input variable coefficient_ring: Remove trailing full stop.
  1. BTermMonoid._create_element_: Replace TESTS: by TESTS::.

New items:

  1. GenericGrowthElement._find_minimum_: replace """" by """ at the beginning of the docstring
  1. GenericGrowthElement._find_minimum_, MonomialGrowthElement._find_minimum_: I suggest to change the description of valid_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"
  1. GenericGrowthElement._find_minimum_: remove trailing full stop from output section.
  1. GenericGrowthElement._find_minimum_, MonomialGrowthElement._find_minimum_: Add full stop to one-sentence description.
  1. MonomialGrowthElement._find_minimum_: replace next(iter(valid_from.values())) by something resembling to valid_from[self.variable_names()[0]] (with some extra care when variable_names() is the empty tuple).
  1. (33. ctd) MonomialGrowthElement._find_minimum_: if self._is_lt_one_(), then an exception (perhaps DecreasingGrowthElementError derived from ValueError) should be raised.
  1. BTerm.absorb: I suggest to replace self.growth._lt_(other.growth) by self.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.
  1. BTerm.absorb: I suggest to replace growth_new = max(self.growth, other.growth) by growth_new = self.growth (because this is ensured anyway) or to remove the variable growth_new completely.
  1. BTerm.absorb: I think that the variable names coeff_max and coeff_min are not very clear; so I proposed to remove them completely, they can be replaced by self.coefficient and other.coefficient, respectively.
  1. GenericGrowthElement._find_minimum_, MonomialGrowthElement._find_minimum_: add at least one test (all functions in SageMath must be tested)

comment:25 Changed 2 months ago by dkrenn

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 2 months ago by git

  • Commit changed from 78c408f9b6f7215e7a38aad23bcbd6cfd925f097 to 06128c1aee93d70470bf47971cb8f68875ce6947

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

d4cfa7cItem 47 and 48. Remove trailing full stop in BTermMonoid description and replace TESTS: with TESTS:: int BTermMonoid._create_element_
4666a16Items 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 one-sentence description. MonomialGrowthElement._find_minimum_: new valid_from description, added full stop to one-sentence description
7109144Items 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.
2146cfeImplemented BTerm._coerce_map_from_
9da7578Item 21. BTerms are allowed to absorb exact terms.
11d56f4Item 33, 53, 54 and 58.
676a401Item 43. Removed valid_from lines from BTerm.can_absorb().
fec2942Item 24. Fixed checking of variables when creating BTerms.
ed3b215Item 8. Added BTerm.explanation() with an explanatory string for what valid_from really means
06128c1Item 27. BTerm._repr_ added a test with multiple entries of values_from

comment:27 Changed 2 months ago by gh-thhagelmayer

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 follow-up: Changed 2 months ago by cheuberg

  • 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 of BTerm). 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 in BTerm.can_absorb anymore. And handling keys of valid_from is now done in BTerm._absorb_. So I consider 16 to be closed.

Thus as far as I am concerned, the following items are open from previous rounds:

  1. example for valid_from: It would be great to have an example where the parametervalid_from is explained within this concrete situation.
  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms.

New items:

  1. BTerm._coerce_map_from_: I think that the coefficient ring of S should also coerce into the coefficient ring of self (This is not mentioned in the Note box, but I think this is mentioned in some of the other term monoids.)
  1. DecreasingGrowthElementError: docstring: I think that MonomialGrowthElement should be replaced by GenericGrowthElement (at some stage, we will also raise this error in other situations than MonomialGrowthElement and I assume that we will then not be aware of the fact that this docstring needs corrections.)
  1. MonomialGrowthElement._find_minimum_: typo "less then one" -> "less than one"
  1. MonomialGrowthElement._find_minimum_: I think that if not len(self.variable_names()): could be stated as an assert 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 tuple self.variable_names() instead of computing the length first.
  1. BTerm.explanation: Replace """" by """.

comment:29 in reply to: ↑ 28 Changed 2 months ago by dkrenn

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 follow-up: Changed 2 months ago by behackl

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 non-optional 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:

  1. Make valid_from an optional keyword argument in BTermMonoid._create_element_ with default value None, and add something along the lines of
    if 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 like BT(x^2, 5) wouldn't error out although valid_from was not specified; it would default to creating a BTerm which is assumed to be valid for all x >= 0.
  1. Adapt GenericTermMonoid._element_constructor_ and explicitly ask in the elif-branch starting on line 1809 whether self is an instance of BTermMonoid – and if so, construct the corresponding valid_from dictionary just like in the first case, then call self._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 interface-friendlier 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 2 months ago by git

  • Commit changed from 06128c1aee93d70470bf47971cb8f68875ce6947 to 395fd73951c8f30522c0f4bef1ac60ddcf5af540

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

4101e7bItem 8 and 63. Removed BTerm.explanation. Moved the examples for valid_from into the docstring of BTerm.
7d02db8Item 60. Replaced MonomialGrowthElement by GenericGrowthElement in DecreasingGrowthElementError.
bf39a80Items 61 and 62. Fixed typo then -> than. Changed calculating the length of the tuple, to test for truthiness of the tuple self.variables().
b75cb35Item 59. BTerm._coerce_map_from_ the coefficient ring of S also coerces into the coefficient ring of self.
395fd73Added check and doctests for logarithmic monomials in MonomialGrowthElement._find_minimum_()

comment:32 Changed 2 months ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review

I have addressed items 8, 59, 60, 61 and 62.

comment:33 Changed 2 months ago by dkrenn

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

comment:34 follow-ups: Changed 2 months ago by dkrenn

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

  1. Use the same description of valid_from everywhere, e.g. the one of BTerm:
    +    - ``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
  1. "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 EXAMPLES-section; then it can also be checked that they work properly.
  1. I suggest to put the @experimental-decorator to the monoid.
  1. Consider putting the experimental-warning in the docstring at the top of the file; maybe in the Various-section. This FutureWarning will otherwise be somewhere in the middle due to #32229.
  1. In __init__, the code
            try:
                coefficient = parent.coefficient_ring(coefficient)
    	...
    
    seems to be unneccessary. This is done in the super-class. I think, the line
            super().__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 super-class init.

Moreover, just seen that in TermWithCoefficient.__init__ the super should (for consistency) be moved to the top.

  1. The code
    self.valid_from = valid_from
    
    should only be done after everything is checked, not before.
  1. In _absorb_, the doctest
                sage: 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.
  1. In _absorb_, the line
            if self.growth < other.growth:
    
    should rather be
            if not (self.growth >= other.growth):
    
    right?
  1. In _absorb_, I think instead of
            union = {**self.valid_from, **other.valid_from}
            for variable_name in union:
    
    the code
    	for variable_name in set().union(self.valid_from.keys(), other.valid_from.keys()):
    
    would be clearer.
  1. In _create_element_, is the idea of the test
        sage: 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:

3498449Trac #31933 review: unify spelling B-term
1933586Trac #31933 review: simplfy doctests by using factory
77717e4Trac #31933 review: minor changes to docstrings etc
Last edited 2 months ago by dkrenn (previous) (diff)

comment:35 in reply to: ↑ 34 ; follow-up: Changed 2 months ago by dkrenn

Replying to dkrenn:

Thanks you for this code; it looks quite good already; I have a couple of minor comments:


New commits:

3498449Trac #31933 review: unify spelling B-term
1933586Trac #31933 review: simplfy doctests by using factory
77717e4Trac #31933 review: minor changes to docstrings etc

I've added a couple of very minor reviewer commits (seemed easier than writing these up; please have a look).

Last edited 2 months ago by dkrenn (previous) (diff)

comment:36 in reply to: ↑ 30 Changed 2 months ago by dkrenn

Replying to behackl:

FWIW, I just had a look at the problem with conversion.

Conversion is now handled in #32229.

comment:37 Changed 2 months ago by dkrenn

I did not check what from <= 63 is still open and what not.

comment:38 in reply to: ↑ 35 Changed 2 months ago by gh-thhagelmayer

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:

3498449Trac #31933 review: unify spelling B-term
1933586Trac #31933 review: simplfy doctests by using factory
77717e4Trac #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 ; follow-up: Changed 2 months ago by gh-thhagelmayer

Replying to dkrenn:

  1. In _create_element_, is the idea of the test
        sage: 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?

comment:40 in reply to: ↑ 34 ; follow-up: Changed 2 months ago by gh-thhagelmayer

Replying to dkrenn:

  1. "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 EXAMPLES-section; 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 B-term.

comment:41 Changed 2 months ago by gh-thhagelmayer

  • Dependencies set to #32153

comment:42 in reply to: ↑ 39 Changed 2 months ago by dkrenn

Replying to gh-thhagelmayer:

Replying to dkrenn:

  1. In _create_element_, is the idea of the test
        sage: 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 ; follow-up: Changed 2 months ago by dkrenn

Replying to gh-thhagelmayer:

Replying to dkrenn:

  1. "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 EXAMPLES-section; 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 B-term.

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 B-terms 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)

    B-terms are, for example,
    ::

        sage: T(x^2, coefficient=5, valid_from={'x': 3})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

    a term whose absolute value is bounded by `5|x|^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 `42|x|^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 B-term 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 `5|x|^2` for `|x| >= 3` (see below for the actual example),

    - bounded by `42|x|^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})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

What do you think?

comment:44 Changed 2 months ago by gh-thhagelmayer

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

comment:45 in reply to: ↑ 43 ; follow-up: Changed 2 months ago by gh-thhagelmayer

  • Commit changed from 77717e4d71091ad09810b0fdc08133b84c17b046 to 50a06cb4e33f149cc6b187f168d6322913b16fdd

Replying to dkrenn:

Replying to gh-thhagelmayer:

Replying to dkrenn:

  1. "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 EXAMPLES-section; 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 B-term.

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 B-terms 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)

    B-terms are, for example,
    ::

        sage: T(x^2, coefficient=5, valid_from={'x': 3})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

    a term whose absolute value is bounded by `5|x|^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 `42|x|^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 B-term 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 `5|x|^2` for `|x| >= 3` (see below for the actual example),

    - bounded by `42|x|^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})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

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

b8868ebItem 70. Remove wrong output and mark the doctest with # not tested.
c2d9603Items 71 and 72. Small corrections in BTerm._absorb_ for better readability.
091271dItem 67. Moved WARNING-Block to the Various section for better visibility.
af7ca87Refactor ExactTerm._repr_ to TermWithCoefficient._product_repr_
f3b1c46Revert TermWithCoefficient._repr_ to be more descriptive.
7fb17cerename _product_repr_ to _repr_product_
7af743fadded docstring to TermWithCoefficient._repr_product_
1db8a42Changed the call to the product representation in ExactTerm._repr_ to self._repr_product_(latex=latex)
c1e50c4Merge remote-tracking branch 'trac/u/gh-thhagelmayer/refactor__repr__of_some_terms' into t/31933/initial_support_for_bterms
50a06cbTrac #31933 Item 67. Fix WARNING-Block.

comment:46 follow-up: Changed 2 months ago by git

  • Commit changed from 50a06cb4e33f149cc6b187f168d6322913b16fdd to 8e5e0e4ce291ccf71b5a78748541725f75e92fd7

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

230bfe7Trac #31933. Changed BTerm._repr_ to use self._repr_product and make the output shorter and fix doctests.
b28061eTrac #31933. Addded latex representation to BTerm._repr_
eec9024Trac #31933. Item 66. Move experimental tag to BTermMonoid.
8082d47Trac #31933. Item 73. Remove doctest which will be impossible after #32215.
8e5e0e4Trac #31933. Item 65. Modify examples with explanation of valid_from to test the examples.

comment:47 Changed 2 months ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review

I worked on items 64-73 and merged #32153 into this ticket.

A review would be appreciated.

comment:48 Changed 8 weeks ago by git

  • Commit changed from 8e5e0e4ce291ccf71b5a78748541725f75e92fd7 to 76d9bcc05abf72d45fe383e5eac89c3142a87c91

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

76d9bccTrac #31933. Fix docbuild issue

comment:49 Changed 8 weeks ago by dkrenn

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 B-term (9 days ago) [Daniel Krenn]

comment:50 in reply to: ↑ 34 Changed 8 weeks ago by dkrenn

Replying to dkrenn:

  1. Consider putting the experimental-warning in the docstring at the top of the file; maybe in the Various-section. This FutureWarning will otherwise be somewhere in the middle due to #32229.
091271dItem 67. Moved WARNING-Block 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 experimental-warning at the top, so something like:

Various
=======

.. WARNING::

    The code for :class:`B-Terms <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 8 weeks ago by dkrenn

Replying to git:

b28061eTrac #31933. Addded latex representation to BTerm._repr_
+        if latex:
+            valid_from_string = ''.join(f'{variable} >= {value}, '
+                                        for variable, value in self.valid_from.items())

I think it should be ', '.join(f'{variable} >= {value}' to avoid the trailing comma.

comment:52 in reply to: ↑ 45 Changed 8 weeks ago by dkrenn

Replying to gh-thhagelmayer:

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 B-terms 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)

    B-terms are, for example,
    ::

        sage: T(x^2, coefficient=5, valid_from={'x': 3})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

    a term whose absolute value is bounded by `5|x|^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 `42|x|^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 B-term 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 `5|x|^2` for `|x| >= 3` (see below for the actual example),

    - bounded by `42|x|^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})
        B-Term with coefficient 5, growth x^2 and valid for x >= 3

What 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 SageMath-code before the input block and the input block is quite late. Maybe we should discuss this once more.

comment:53 Changed 7 weeks ago by git

  • Commit changed from 76d9bcc05abf72d45fe383e5eac89c3142a87c91 to 5a3c5b1d928fdf1f32d9f57f77bf866e814b9ae6

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

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.

comment:54 Changed 7 weeks ago by gh-thhagelmayer

Thank you for your comments. I merged the missing reviewer commits and corrected the small issues.


New commits:

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.

comment:55 Changed 7 weeks ago by cheuberg

  • 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

  1. BTerm.can_absorb: I think that BTerms should be able to absorb exact terms

on the list, just for the record.

New items:

  1. (f'up to 8, 65) BTerm docstring: "all functions which (in absolute value) is bounded" -> "all functions which (in absolute value) are bounded".
  1. 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.
  1. (f'up to 64) site-packages/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 for find_minimum ("The bound implied by this term is valid" is good for BTerms, but not for _find_minimum_). I suggest to revert the changes made in 1c1ebd41 in growth_group.py only.
  1. 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.
  1. BTerm docstring: Missing empty line after "We revisit the example from the introduction::" leads to destroyed formatting of the examples
  1. In the various section: remove "L-terms" from the todo box of things that need to be implemented, add your name to authors and acknowledgement.
  1. BTerm._latex_: Replace >= by \ge.
  1. BTerm docstring: The old example BT_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.
  1. 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 follow-ups: Changed 6 weeks ago by git

  • Commit changed from 5a3c5b1d928fdf1f32d9f57f77bf866e814b9ae6 to c8c8ec486fad105cc65e68b1889ff95575b49c97

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

bf880c7Trac #31933. Item 74 fix BTerm docstring.
447f64fTrac #31933. Item 75. Add more meaningful Error message
2a86c51Trac #31933. Item 81: remove an example
2194881Trac #31933. Item 78: add missing empty line
b8e7b86Trac #31933. Item 82: Split examples block into three parts to have clearer explanations without scrolling.
e494cb7Trac #31933. Item 80: BTerm._latex_: replace >= by \ge.
04c07e4Trac #31933. Item 79: Add Author and acknowledgment. Remove L-terms from todo box.
9f49586Trac #31933. Item 77: move hint to a more meaningful line
c8c8ec4Trac #31933. Item 76: revert GenericGrowthElement._find_minimum_ Input explanation introduced in 1c1ebd41. Add missing empty line.

comment:57 Changed 6 weeks ago by cheuberg

Thank you. I still have a few rather trivial comments.

  1. (f'up to 81): BTerm.__init__ docstring: MonomialGrowthGroup is imported, but unused.
  1. 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
  1. BTerm.__init__: Replace >= by \ge in all TeX formatted code
  1. (f'up to 80) BTerm._latex_: I think that f'{variable} \ge {value}' should be fr'{variable} \ge {value}' or f'{variable} \\ge {value}' to avoid the warning 4162: DeprecationWarning: invalid escape sequence \g
  1. (f'up to 76): MonomialGrowthElement._find_minimum_: please use the same description for the valid_from as in GenericGrowthElement._find_minimum_.
  1. (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 weeks ago by git

  • Commit changed from c8c8ec486fad105cc65e68b1889ff95575b49c97 to 2e46c20aee507515cdb19f3847d0a337f840b535

Branch pushed to git repo; I updated commit sha1. 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

comment:59 Changed 6 weeks ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review

I have worked on items 83, 84, 85, 86, 87, 88.

comment:60 Changed 6 weeks ago by cheuberg

  • Milestone changed from sage-9.4 to sage-9.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 weeks ago by dkrenn

Replying to git:

c8c8ec4Trac #31933. Item 76: revert GenericGrowthElement._find_minimum_ Input explanation introduced in 1c1ebd41. Add missing empty line.
  1. 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 weeks ago by dkrenn

Replying to git:

b8e7b86Trac #31933. Item 82: Split examples block into three parts to have clearer explanations without scrolling.
  1. I suggest to replace
    +    This is a term bounded by `5|x|^2` for `|x| >= 3`.
    +    ::
    

by

+    This is a term bounded by `5|x|^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 weeks ago by dkrenn

  • 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 weeks ago by dkrenn

Just had a look at the code because of #32214 and found two more very small things:

  1. I think, we should remove "big" in the only occurrence
    +    Parent for asymptotic big `B`-terms.
    

to stay consistent.

  1. For consistency, we should replace the two
    `B`-term
    

by B-term.

comment:65 Changed 5 weeks ago by git

  • Commit changed from 2e46c20aee507515cdb19f3847d0a337f840b535 to f631489e308d859ee22ff2464cc28724f1789782

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

f631489Trac #31933. Items: 90, 91, 92: minor corrections in the documentation

comment:66 Changed 5 weeks ago by tscrim

  • Keywords gsoc2021 added; gsoc21 removed

comment:67 Changed 5 weeks ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review

comment:68 Changed 4 weeks ago by behackl

  • 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 3 weeks ago by vbraun

  • Branch changed from u/gh-thhagelmayer/initial_support_for_bterms to f631489e308d859ee22ff2464cc28724f1789782
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.