#32153 closed enhancement (fixed)
Refactor _repr_ of some Terms
Reported by:  ghthhagelmayer  Owned by:  

Priority:  major  Milestone:  sage9.4 
Component:  asymptotic expansions  Keywords:  gsoc2021, asymptotics 
Cc:  cheuberg, behackl, dkrenn  Merged in:  
Authors:  Thomas Hagelmayer  Reviewers:  Benjamin Hackl, Daniel Krenn 
Report Upstream:  N/A  Work issues:  
Branch:  c11b2f4 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
Refactor ExactTerm?._repr_ and TermWithCoefficient?._repr_
Change History (19)
comment:1 Changed 11 months ago by
 Branch set to u/ghthhagelmayer/refactor__repr__of_some_terms
comment:2 Changed 11 months ago by
 Commit set to af7ca87f87075d79b9c7ccd2e59f8fcb8205e1e8
 Status changed from new to needs_review
comment:3 Changed 11 months ago by
I'll investigate the doctest failures later. The refactor looks good to me in general!
I have a slight preference for keeping the old TermWithCoefficient._repr_
.
comment:4 Changed 11 months ago by
 Status changed from needs_review to needs_work
The failing doctests are caused by two representation strings that have not been adapted yet:

src/sage/rings/asymptotic/term_monoid.py
a b class GenericTerm(MultiplicativeGroupElement): 1119 1119 sage: T.an_element().is_little_o_of_one() 1120 1120 Traceback (most recent call last): 1121 1121 ... 1122 NotImplementedError: Cannot check whether Term with coefficient 1/2 and growthx1122 NotImplementedError: Cannot check whether Term with coefficient 1/2*x 1123 1123 is o(1) in the abstract base class 1124 1124 Generic Term Monoid x^ZZ with (implicit) coefficients in Rational Field. 1125 1125 """ … … class TermWithCoefficient(GenericTerm): 3015 3015 sage: T(G.an_element(), coefficient=CIF(RIF(1,1), RIF(1,1)))._calculate_pow_(I) 3016 3016 Traceback (most recent call last): 3017 3017 ... 3018 ArithmeticError: Cannot take Term with coefficient 0.? + 0.?*I and3019 growth zto the exponent I in Generic Term Monoid z^ZZ with3018 ArithmeticError: Cannot take Term with coefficient (0.? + 0.?*I)*z 3019 to the exponent I in Generic Term Monoid z^ZZ with 3020 3020 (implicit) coefficients in Complex Interval Field with 3021 3021 53 bits of precision since its coefficient 0.? + 0.?*I
I also want to briefly clarify: semantically, I do think that "Term with coefficient C and growth G" is easier to read than "Term with coefficient C*G", but at the same time these generic terms are not really userfacing. I'll leave it up to you which version you choose; the important thing is that the code for formatting the coefficientgrowth product is now easily accessible via _product_repr_
.
comment:5 Changed 11 months ago by
 Commit changed from af7ca87f87075d79b9c7ccd2e59f8fcb8205e1e8 to f3b1c46239e44d3bbadd58980056fe8797684e8a
Branch pushed to git repo; I updated commit sha1. New commits:
f3b1c46  Revert TermWithCoefficient._repr_ to be more descriptive.

comment:6 Changed 11 months ago by
 Status changed from needs_work to needs_review
I reverted back the changes to TermWithCoefficient._repr_
, because I think it is more descriptive. For eg:
Term with coefficient 1 and growth x
vs.
Term with coefficient x
comment:7 Changed 11 months ago by
I guess, we should use _repr_product_
instead of _product_repr_
because
dakrenn@nops:~$ sage grep " _[azAZ].*_repr_*("  wc l 50 dakrenn@nops:~$ sage grep " _repr_[azAZ].*("  wc l 338
comment:8 followup: ↓ 11 Changed 11 months ago by
+ return self._product_repr_(latex)
I think we should simply pass on all (keyword) arguments, i.e.
+ return self._product_repr_(**kwds)
comment:9 Changed 11 months ago by
 Reviewers set to Daniel Krenn
 Status changed from needs_review to needs_work
Apart from the above, code LGTM.
comment:10 Changed 11 months ago by
 Reviewers changed from Daniel Krenn to Benjamin Hackl, Daniel Krenn
Great! If you have a look at what the patchbot reports, you can see that it complains about the coverage going down.
Please add a docstring and a simple doctest for TermWithCoefficient._product_repr_
.
comment:11 in reply to: ↑ 8 ; followup: ↓ 12 Changed 11 months ago by
Replying to dkrenn:
+ return self._product_repr_(latex)I think we should simply pass on all (keyword) arguments, i.e.
+ return self._product_repr_(**kwds)
But that would mean that the header of _repr_
needs to be something like def _repr_(self, **kwds)
instead of the more explicit def _repr_(self, latex=...)
. I don't think that is an actual improvement, can you elaborate?
comment:12 in reply to: ↑ 11 ; followup: ↓ 13 Changed 11 months ago by
Replying to behackl:
Replying to dkrenn:
+ return self._product_repr_(latex)I think we should simply pass on all (keyword) arguments, i.e.
+ return self._product_repr_(**kwds)But that would mean that the header of
_repr_
needs to be something likedef _repr_(self, **kwds)
instead of the more explicitdef _repr_(self, latex=...)
. I don't think that is an actual improvement, can you elaborate?
Ideally, I would like to do
sage: class A(object): ....: def sun(self): ....: pass ....: class B(A): ....: rain = sun
but this raises
NameError: name 'sun' is not defined
so I thought the next best thing is to pass on all arguments by **kwds
. However, I agree that explicit might be better in this case, so keeping the methoddefinition and using
return self._product_repr_(latex=latex)
seems to be a good compromise.
comment:13 in reply to: ↑ 12 Changed 11 months ago by
Replying to dkrenn:
[...], so keeping the methoddefinition and using
return self._product_repr_(latex=latex)seems to be a good compromise.
+1, passing the keyword argument latex
explicitly is a good idea. I'm also happy with this. This means that there are three minor things to do here:
 Rename
_product_repr_
to_repr_product_
,  Add a short docstring with a doctest (you can use the same doctest as for
TermWithCoefficient._repr_
),  Change the call to the product representation in
ExactTerm._repr_
toself._product_repr_(latex=latex)
.
comment:14 Changed 11 months ago by
 Commit changed from f3b1c46239e44d3bbadd58980056fe8797684e8a to 1db8a42d0288607f913c1ea279e295034e00c3d0
comment:15 followup: ↓ 17 Changed 11 months ago by
 Status changed from needs_work to needs_review
 Rename
_product_repr_
to_repr_product_
,  Add a short docstring with a doctest (you can use the same doctest as for
TermWithCoefficient._repr_
),  Change the call to the product representation in
ExactTerm._repr_
toself._product_repr_(latex=latex)
.
I addressed the above three items in the latest commits.
comment:16 Changed 10 months ago by
 Branch changed from u/ghthhagelmayer/refactor__repr__of_some_terms to u/dkrenn/refactor__repr__of_some_terms
comment:17 in reply to: ↑ 15 Changed 10 months ago by
 Commit changed from 1db8a42d0288607f913c1ea279e295034e00c3d0 to c11b2f46051c9438d90bf45c69d7437c421b3b48
 Status changed from needs_review to positive_review
Replying to ghthhagelmayer:
 Rename
_product_repr_
to_repr_product_
, Add a short docstring with a doctest (you can use the same doctest as for
TermWithCoefficient._repr_
), Change the call to the product representation in
ExactTerm._repr_
toself._product_repr_(latex=latex)
.I addressed the above three items in the latest commits.
Thank you very much. Fixed one missing full stop. LGTM
New commits:
c11b2f4  Trac #32153: fix missing full stop

comment:18 Changed 10 months ago by
 Branch changed from u/dkrenn/refactor__repr__of_some_terms to c11b2f46051c9438d90bf45c69d7437c421b3b48
 Resolution set to fixed
 Status changed from positive_review to closed
comment:19 Changed 9 months ago by
 Commit c11b2f46051c9438d90bf45c69d7437c421b3b48 deleted
 Keywords gsoc2021 added; gsoc21 removed
This has now the refactored
TermWithCoefficient._product_repr_
.I get two doctest failures. Something to do with
<BLANKLINE>
.New commits:
Refactor ExactTerm._repr_ to TermWithCoefficient._product_repr_