Opened 10 months ago
Closed 9 months ago
#32214 closed defect (fixed)
Asymptotic Term Monoids: unify repr
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage9.5 
Component:  asymptotic expansions  Keywords:  
Cc:  cheuberg, behackl, ghthhagelmayer  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  f76ffc6 (Commits, GitHub, GitLab)  Commit:  f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57 
Dependencies:  Stopgaps: 
Description
TermWithCoefficientMonid
does not have a _repr_
; add this and unify _repr_
of GenericTermMonoid
.
Change History (13)
comment:1 Changed 10 months ago by
 Branch set to u/dkrenn/termmonoidsunifyrepr
comment:2 Changed 10 months ago by
 Commit set to a0b6a1e235cb112b5a00b810cfd9cd0c65ea2521
 Status changed from new to needs_review
comment:3 Changed 10 months ago by
 Cc cheuberg behackl ghthhagelmayer added
comment:4 followup: ↓ 5 Changed 10 months ago by
 Status changed from needs_review to needs_info
There is one thing I'd briefly like to discuss: The representation strings introduced and modified here start with XMonoid ...
, where X
is the name of the corresponding element class. If we are unifying all of these, I think that the same should hold for OTermMonoid._repr_
, which should produce a string starting with OTermMonoid
, instead of the current OTerm Monoid
.
Other than that, these changes LGTM.
comment:5 in reply to: ↑ 4 Changed 10 months ago by
Replying to behackl:
There is one thing I'd briefly like to discuss: The representation strings introduced and modified here start with
XMonoid ...
, whereX
is the name of the corresponding element class. If we are unifying all of these, I think that the same should hold forOTermMonoid._repr_
, which should produce a string starting withOTermMonoid
, instead of the currentOTerm Monoid
.
I am against this change. I think there are two ways to write the monoids, either XMonoid
where X
is a class name or a more verbose name if possible, which is the case for OTerm Monoid
(and also for BTerm Monoid
on #31933.
We could, if we like, however unify the dashs by writing GenericTerm Monoid
etc (without dash), but I do not have a strong preference for doing this.
comment:6 followup: ↓ 8 Changed 10 months ago by
We could, if we like, however unify the dashs by writing
GenericTerm Monoid
etc (without dash), but I do not have a strong preference for doing this.
I feel that this ticket currently introduces a weird inconsistency with respect to the representation strings. I don't really mind the direction in which this is changed to, but I am against leaving it asis. Your suggestion with the removal of the dashes before Monoid
is fine for me (i.e., TermWithCoefficient Monoid
and OTerm Monoid
).
comment:7 Changed 10 months ago by
 Commit changed from a0b6a1e235cb112b5a00b810cfd9cd0c65ea2521 to 2f9c6b6eeb3f51ee0b919b8ab6da2edd67fae3ec
Branch pushed to git repo; I updated commit sha1. New commits:
2f9c6b6  Trac #32214: unify dash in monoid repr

comment:8 in reply to: ↑ 6 Changed 10 months ago by
 Status changed from needs_info to needs_review
Replying to behackl:
We could, if we like, however unify the dashs by writing
GenericTerm Monoid
etc (without dash), but I do not have a strong preference for doing this.I feel that this ticket currently introduces a weird inconsistency with respect to the representation strings. I don't really mind the direction in which this is changed to, but I am against leaving it asis. Your suggestion with the removal of the dashes before
Monoid
is fine for me (i.e.,TermWithCoefficient Monoid
andOTerm Monoid
).
Ok. Changed.
comment:9 Changed 10 months ago by
 Branch changed from u/dkrenn/termmonoidsunifyrepr to u/behackl/termmonoidsunifyrepr
comment:10 Changed 10 months ago by
 Commit changed from 2f9c6b6eeb3f51ee0b919b8ab6da2edd67fae3ec to f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57
 Reviewers set to Benjamin Hackl
Thanks! I've pushed two minor reviewer commits, feel free to crossreview and set this to positive_review
in case you are satisfied. (Otherwise I'll do so myself as soon as the patchbot is done.)
New commits:
5ef7ece  adapted TermWithCoefficientMonoid._repr_ summary

f76ffc6  simplified doctest via DefaultTermMonoidFactory

comment:11 Changed 10 months ago by
 Status changed from needs_review to positive_review
comment:12 Changed 9 months ago by
 Milestone changed from sage9.4 to sage9.5
comment:13 Changed 9 months ago by
 Branch changed from u/behackl/termmonoidsunifyrepr to f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
add _repr_ for TermWithCoefficientMonoid
update and unify GenericTermMonoid _repr_