Opened 11 months ago

Closed 10 months ago

Last modified 9 months ago

#32153 closed enhancement (fixed)

Refactor _repr_ of some Terms

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

Status badges

Description

Refactor ExactTerm?._repr_ and TermWithCoefficient?._repr_

Change History (19)

comment:1 Changed 11 months ago by gh-thhagelmayer

  • Branch set to u/gh-thhagelmayer/refactor__repr__of_some_terms

comment:2 Changed 11 months ago by gh-thhagelmayer

  • Commit set to af7ca87f87075d79b9c7ccd2e59f8fcb8205e1e8
  • Status changed from new to needs_review

This has now the refactored TermWithCoefficient._product_repr_.

I get two doctest failures. Something to do with <BLANKLINE>.

File "src/sage/rings/asymptotic/term_monoid.py", line 1119, in sage.rings.asymptotic.term_monoid.GenericTerm.is_little_o_of_one
File "src/sage/rings/asymptotic/term_monoid.py", line 3015, in sage.rings.asymptotic.term_monoid.TermWithCoefficient._calculate_pow_

New commits:

af7ca87Refactor ExactTerm._repr_ to TermWithCoefficient._product_repr_

comment:3 Changed 11 months ago by behackl

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 behackl

  • 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): 
    11191119            sage: T.an_element().is_little_o_of_one()
    11201120            Traceback (most recent call last):
    11211121            ...
    1122             NotImplementedError: Cannot check whether Term with coefficient 1/2 and growth x
     1122            NotImplementedError: Cannot check whether Term with coefficient 1/2*x
    11231123            is o(1) in the abstract base class
    11241124            Generic Term Monoid x^ZZ with (implicit) coefficients in Rational Field.
    11251125        """
    class TermWithCoefficient(GenericTerm): 
    30153015            sage: T(G.an_element(), coefficient=CIF(RIF(-1,1), RIF(-1,1)))._calculate_pow_(I)
    30163016            Traceback (most recent call last):
    30173017            ...
    3018             ArithmeticError: Cannot take Term with coefficient 0.? + 0.?*I and
    3019             growth z to the exponent I in Generic Term Monoid z^ZZ with
     3018            ArithmeticError: Cannot take Term with coefficient (0.? + 0.?*I)*z
     3019            to the exponent I in Generic Term Monoid z^ZZ with
    30203020            (implicit) coefficients in Complex Interval Field with
    30213021            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 user-facing. I'll leave it up to you which version you choose; the important thing is that the code for formatting the coefficient-growth product is now easily accessible via _product_repr_.

comment:5 Changed 11 months ago by git

  • Commit changed from af7ca87f87075d79b9c7ccd2e59f8fcb8205e1e8 to f3b1c46239e44d3bbadd58980056fe8797684e8a

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

f3b1c46Revert TermWithCoefficient._repr_ to be more descriptive.

comment:6 Changed 11 months ago by gh-thhagelmayer

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

I guess, we should use _repr_product_ instead of _product_repr_ because

dakrenn@nops:~$ sage -grep " _[a-zA-Z].*_repr_*(" | wc -l
50
dakrenn@nops:~$ sage -grep " _repr_[a-zA-Z].*(" | wc -l
338

comment:8 follow-up: Changed 11 months ago by dkrenn

+        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 dkrenn

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

  • 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 ; follow-up: Changed 11 months ago by 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 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 ; follow-up: Changed 11 months ago by dkrenn

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 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?

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 method-definition 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 behackl

Replying to dkrenn:

[...], so keeping the method-definition 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:

  1. Rename _product_repr_ to _repr_product_,
  2. Add a short docstring with a doctest (you can use the same doctest as for TermWithCoefficient._repr_),
  3. Change the call to the product representation in ExactTerm._repr_ to self._product_repr_(latex=latex).

comment:14 Changed 11 months ago by git

  • Commit changed from f3b1c46239e44d3bbadd58980056fe8797684e8a to 1db8a42d0288607f913c1ea279e295034e00c3d0

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

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)

comment:15 follow-up: Changed 11 months ago by gh-thhagelmayer

  • Status changed from needs_work to needs_review
  1. Rename _product_repr_ to _repr_product_,
  2. Add a short docstring with a doctest (you can use the same doctest as for TermWithCoefficient._repr_),
  3. Change the call to the product representation in ExactTerm._repr_ to self._product_repr_(latex=latex).

I addressed the above three items in the latest commits.

comment:16 Changed 10 months ago by dkrenn

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

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

  • Commit changed from 1db8a42d0288607f913c1ea279e295034e00c3d0 to c11b2f46051c9438d90bf45c69d7437c421b3b48
  • Status changed from needs_review to positive_review

Replying to gh-thhagelmayer:

  1. Rename _product_repr_ to _repr_product_,
  2. Add a short docstring with a doctest (you can use the same doctest as for TermWithCoefficient._repr_),
  3. Change the call to the product representation in ExactTerm._repr_ to self._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:

c11b2f4Trac #32153: fix missing full stop

comment:18 Changed 10 months ago by vbraun

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

  • Commit c11b2f46051c9438d90bf45c69d7437c421b3b48 deleted
  • Keywords gsoc2021 added; gsoc21 removed
Note: See TracTickets for help on using tickets.