Opened 10 months ago

Closed 5 months ago

#32215 closed enhancement (fixed)

Asymptotic Term Monoids: refactor element construction

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-9.5
Component: asymptotic expansions Keywords: gsoc2021
Cc: cheuberg, behackl, gh-thhagelmayer Merged in:
Authors: Daniel Krenn Reviewers: Benjamin Hackl
Report Upstream: N/A Work issues:
Branch: 89ad9d2 (Commits, GitHub, GitLab) Commit: 89ad9d2cbd4aed3a7c0b1065766c03727c754e01
Dependencies: #32214 Stopgaps:

Status badges

Description

In #31933 conversion fails. In order to fix this properly, a refactor of the conversion process seems the best solution. This ticket does this (without fixing the open issue on #31933 which will be a follow up).

Change History (37)

comment:1 Changed 10 months ago by dkrenn

  • Branch set to u/dkrenn/refactor-element-construction-term-monoids

comment:2 Changed 10 months ago by dkrenn

  • Authors set to Daniel Krenn
  • Commit set to c805fc19b60e9ac4ff028f63d4c687cb4d50bca2
  • Dependencies set to #32214
  • Status changed from new to needs_review

New commits:

9061510add _repr_ for TermWithCoefficientMonoid
a0b6a1eupdate and unify GenericTermMonoid _repr_
4062f9arefactor element construction in term monoids
c805fc1Trac #32215: deprecate positional "coefficient" argument

comment:3 Changed 10 months ago by dkrenn

Review recommendation: commit-wise

comment:4 Changed 10 months ago by dkrenn

  • Cc cheuberg behackl gh-thhagelmayer added

comment:5 Changed 10 months ago by git

  • Commit changed from c805fc19b60e9ac4ff028f63d4c687cb4d50bca2 to f76b7e7ef42e8cf09301cc38277a11024e210f1d

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

f76b7e7Trac #32215: use defaults of super class

comment:6 Changed 10 months ago by git

  • Commit changed from f76b7e7ef42e8cf09301cc38277a11024e210f1d to c4792672602326a9cc615fb0060d5e09a20d69dd

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

c981805Trac #32215: unify docstrings of .construction
c479267Trac #32215: remove caching from _default_kwds_construction_

comment:7 Changed 10 months ago by git

  • Commit changed from c4792672602326a9cc615fb0060d5e09a20d69dd to 3c5c0f0d88eec1853137f15466f254c378b32b5d

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

3c5c0f0fix coercion related to absorbtion

comment:8 Changed 10 months ago by behackl

  • Branch changed from u/dkrenn/refactor-element-construction-term-monoids to u/behackl/refactor-element-construction-term-monoids

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

  • Commit changed from 3c5c0f0d88eec1853137f15466f254c378b32b5d to 1975a10ac0f58240e195e66cf08c6105ad125a7e
  • Reviewers set to Benjamin Hackl
  • Status changed from needs_review to needs_work

Here are some minor points that should be addressed:

  1. The new exceptions caused by ambiguously passing additional growth and coefficient keyword arguments should be tested.
  1. Is there a particular reason for changing the implementation of OTerm.can_absorb?
  1. The implementations of TermWithCoefficientMonoid._convert_construction_ and ExactTermMonoid._convert_construction_ are identical. I think the tests contained in the docstring of ExactTermMonoid._convert_construction_ should be moved to TermWithCoefficientMonoid._convert_construction_ and the separate implementation of ExactTermMonoid._convert_construction_ should be removed.

Otherwise, the proposed changes look good to me. Here are the changes I've made in my reviewer commits:

  • the cached_method import was unused and is now removed.
  • The EXAMPLES block in some newly added methods used TermMonoidFactory to manually construct a factory, I've changed it to from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid (also see #32032).
  • Language improvements for the error messages and the deprecation warning in GenericTermMonoid._element_constructor.

New commits:

bc7b2bcremoved superfluous import
d7cefd9minor language improvements for docstrings and deprecation warning
1975a10changed doctests of new methods to use DefaultTermMonoidFactory

comment:10 Changed 10 months ago by gh-thhagelmayer

  • Branch changed from u/behackl/refactor-element-construction-term-monoids to u/gh-thhagelmayer/refactor-element-construction-term-monoids

comment:11 Changed 10 months ago by gh-thhagelmayer

  • Commit changed from 1975a10ac0f58240e195e66cf08c6105ad125a7e to 0a506354ea6ec01df3e7ba5b093d5c6e06179393

I have merged #32032 and fixed some doctests.


New commits:

e925c05Merge branch 't/32032/a3bad87abf0a70f85adbcba9b88244b0393a82ad' into t/32215/refactor-element-construction-term-monoids
0a50635Trac #32215: fix doctests

comment:12 in reply to: ↑ 9 Changed 10 months ago by dkrenn

Replying to behackl:

Otherwise, the proposed changes look good to me. Here are the changes I've made in my reviewer commits:

  • the cached_method import was unused and is now removed.
  • The EXAMPLES block in some newly added methods used TermMonoidFactory to manually construct a factory, I've changed it to from sage.rings.asymptotic.term_monoid import DefaultTermMonoidFactory as TermMonoid (also see #32032).
  • Language improvements for the error messages and the deprecation warning in GenericTermMonoid._element_constructor.

LGTM, thanks.

comment:13 Changed 10 months ago by dkrenn

  • Branch changed from u/gh-thhagelmayer/refactor-element-construction-term-monoids to u/dkrenn/refactor-element-construction-term-monoids

comment:14 Changed 10 months ago by git

  • Commit changed from 0a506354ea6ec01df3e7ba5b093d5c6e06179393 to d763d663355c4519128a616fb5f6ce7ebba2cfee

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

d763d66Trac #32215: full coverage of _element_constructor_

comment:15 in reply to: ↑ 9 Changed 10 months ago by dkrenn

Replying to behackl:

  1. The new exceptions caused by ambiguously passing additional growth and coefficient keyword arguments should be tested.

Tested now. Indeed there was code, that could have never been reached. I think we should have full coverage now.

  1. Is there a particular reason for changing the implementation of OTerm.can_absorb?

This was the only place, where the <= of a term was used. However, I think it should be made clear what we compare here ("explicit is better than implicit"), so I switched to comparing the growth directly.

The reason for having <= (and method le) is for being compatible with SageMath's poset class in this perspective. If it wouldn't be for that, I would suggest to deprecate <= in the long run. However, as there is a reason for keeping it, I suggest to, at least, not use it further.

  1. The implementations of TermWithCoefficientMonoid._convert_construction_ and ExactTermMonoid._convert_construction_ are identical. I think the tests contained in the docstring of ExactTermMonoid._convert_construction_ should be moved to TermWithCoefficientMonoid._convert_construction_ and the separate implementation of ExactTermMonoid._convert_construction_ should be removed.

I disagree here. Usually I am in strong favour of removeing code duplicities, however, _convert_construction_ is the one crucial method that all different terms (or their monoids, to be precise) have that do the conversion and making this conversion as explicit as possible helps understanding what is going on. I find it more of an incidient that two different type of terms have the same conversion.

comment:16 Changed 10 months ago by behackl

  • Status changed from needs_work to positive_review

I think we should have full coverage now.

Great, thanks!

I would suggest to deprecate <= in the long run. However, as there is a reason for keeping it, I suggest to, at least, not use it further.

Another example of spiritual deprecation. +1.

I find it more of an incidient that two different type of terms have the same conversion.

Okay, good argument.

This LGTM now, thanks for your efforts!

comment:17 Changed 10 months ago by behackl

I am not sure why the deprecation_number test in the most recent patchbot run failed though. The deprecation introduced here correctly uses 32215.

comment:18 Changed 10 months ago by git

  • Commit changed from d763d663355c4519128a616fb5f6ce7ebba2cfee to 540d08880bdcc5fbda1b2487bce9c74d3d35f27c
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

540d088Merge commit 'c11b2f4' into t/32215/refactor-element-construction-term-monoids

comment:19 Changed 10 months ago by git

  • Commit changed from 540d08880bdcc5fbda1b2487bce9c74d3d35f27c to 62cae10d33ab58abe5b2bbb2388adce8e01d1487

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

62cae10Trac #32215: fix doctest after merge

comment:20 Changed 10 months ago by dkrenn

Merged #32153 and fixed doctest. It's now on 9.4.beta4.

comment:21 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:22 Changed 10 months ago by behackl

  • Status changed from needs_review to positive_review

comment:23 Changed 9 months ago by gh-thhagelmayer

  • Keywords gsoc2021 added

comment:24 Changed 9 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:25 Changed 8 months ago by gh-thhagelmayer

  • Branch changed from u/dkrenn/refactor-element-construction-term-monoids to u/gh-thhagelmayer/refactor-element-construction-term-monoids

comment:26 Changed 8 months ago by gh-thhagelmayer

  • Commit changed from 62cae10d33ab58abe5b2bbb2388adce8e01d1487 to d2eb45dcd993194e9e5c262a5ef3e3d352666a2d
  • Status changed from needs_work to needs_review

I merged #31933 into this ticket and resolved the merge conflict.


New commits:

d2eb45dMerge commit 'f631489' into t/32215/refactor-element-construction-term-monoids

comment:27 Changed 8 months ago by git

  • Commit changed from d2eb45dcd993194e9e5c262a5ef3e3d352666a2d to 5e9887771df00abbdf9827d8a46bf546721ac22b

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

5e98877Restore files which have been accidentally removed in the last commit and prevented building sage.

comment:28 Changed 6 months ago by git

  • Commit changed from 5e9887771df00abbdf9827d8a46bf546721ac22b to dfd5ae49c72effc3d0811f84d9e7a9d346809933

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

6c3af69Merge commit 'f631489' into t/32215/refactor-element-construction-term-monoids
dfd5ae4Trac #32215: fix doctest after merge

comment:29 follow-up: Changed 6 months ago by gh-thhagelmayer

Somwthing went wrong with the last push here, so I went back to the status of u/dkrenn/refactor-element-construction-term-monoids and redid the merge and fixed the doctests. There is still one doctest failing:

File "src/sage/rings/asymptotic/term_monoid.py", line 4698, in sage.rings.asymptotic.term_monoid.BTerm.can_absorb
Failed example:
    t2.absorb(t1)
Expected:
    B(2003/400*x^3, x >= 20)
Got:
...
    DeprecationWarning: Passing 'coefficient' as a positional argument is deprecated; specify it as keyword argument 'coefficient=...'.
    See https://trac.sagemath.org/32215 for details.
    B(2003/400*x^3, x >= 20)

comment:30 in reply to: ↑ 29 ; follow-up: Changed 6 months ago by behackl

Replying to gh-thhagelmayer:

Somwthing went wrong with the last push here, so I went back to the status of u/dkrenn/refactor-element-construction-term-monoids and redid the merge and fixed the doctests. There is still one doctest failing:

File "src/sage/rings/asymptotic/term_monoid.py", line 4698, in sage.rings.asymptotic.term_monoid.BTerm.can_absorb
Failed example:
    t2.absorb(t1)
Expected:
    B(2003/400*x^3, x >= 20)
Got:
...
    DeprecationWarning: Passing 'coefficient' as a positional argument is deprecated; specify it as keyword argument 'coefficient=...'.
    See https://trac.sagemath.org/32215 for details.
    B(2003/400*x^3, x >= 20)

Yes, this should be resolved as well: it comes from https://git.sagemath.org/sage.git/tree/src/sage/rings/asymptotic/term_monoid.py?id=683acc1eaa07fd3375bdd031516cfce30f1e363b#n4753, the coefficient should be passed via keyword argument.

comment:31 Changed 6 months ago by git

  • Commit changed from dfd5ae49c72effc3d0811f84d9e7a9d346809933 to 7e7b5c7bf93c713e3688a94ccb33561092556b83

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

7e7b5c7Trac #32215: fix a doctest. Use **kwds instead of coefficient

comment:32 in reply to: ↑ 30 Changed 6 months ago by gh-thhagelmayer

I fixed the doctest in 7e7b5c7

comment:33 follow-up: Changed 6 months ago by behackl

I've managed to find the problem causing the deprecation_number plugin failure: looking at the implementation of the plugin over at https://github.com/sagemath/sage-patchbot/blob/745ec21b12f3bce03e8f1c8399cba0fb77bdc45c/sage_patchbot/plugins.py#L546, the plugin parses all added lines. If one of those contains deprecation( or deprecated_function_alias(, the code tries to parse the part following afterwards to find the ticket number, and then compares this parsed number to the actual ticket number.

The problem with our code is that we currently have

+            deprecation(
+                32215,
+                "Passing 'coefficient' as a positional argument is deprecated; "
+                "specify it as keyword argument 'coefficient=...'.")

in our diff, so the code fails to properly parse the deprecation number. Changing this addition in term_monoid.py to

+            deprecation(32215,
+                "Passing 'coefficient' as a positional argument is deprecated; "
+                "specify it as keyword argument 'coefficient=...'.")

will fix the failing test.

comment:34 Changed 6 months ago by git

  • Commit changed from 7e7b5c7bf93c713e3688a94ccb33561092556b83 to 89ad9d2cbd4aed3a7c0b1065766c03727c754e01

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

89ad9d2Trac #31125: fix deprecation plugin failure

comment:35 in reply to: ↑ 33 Changed 6 months ago by gh-thhagelmayer

Replying to behackl:

I've managed to find the problem causing the deprecation_number plugin failure: looking at the implementation of the plugin over at https://github.com/sagemath/sage-patchbot/blob/745ec21b12f3bce03e8f1c8399cba0fb77bdc45c/sage_patchbot/plugins.py#L546, the plugin parses all added lines. If one of those contains deprecation( or deprecated_function_alias(, the code tries to parse the part following afterwards to find the ticket number, and then compares this parsed number to the actual ticket number.

The problem with our code is that we currently have

+            deprecation(
+                32215,
+                "Passing 'coefficient' as a positional argument is deprecated; "
+                "specify it as keyword argument 'coefficient=...'.")

in our diff, so the code fails to properly parse the deprecation number. Changing this addition in term_monoid.py to

+            deprecation(32215,
+                "Passing 'coefficient' as a positional argument is deprecated; "
+                "specify it as keyword argument 'coefficient=...'.")

will fix the failing test.

I changed that line accordingly.

comment:36 Changed 6 months ago by behackl

  • Status changed from needs_review to positive_review

This seems fine to me now, thank you! The latest patchbot test failures seem unrelated.

comment:37 Changed 5 months ago by vbraun

  • Branch changed from u/gh-thhagelmayer/refactor-element-construction-term-monoids to 89ad9d2cbd4aed3a7c0b1065766c03727c754e01
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.