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: sage-9.5
Component: asymptotic expansions Keywords:
Cc: cheuberg, behackl, gh-thhagelmayer Merged in:
Authors: Daniel Krenn Reviewers: Benjamin Hackl
Report Upstream: N/A Work issues:
Branch: f76ffc6 (Commits, GitHub, GitLab) Commit: f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57
Dependencies: Stopgaps:

Status badges

Description

TermWithCoefficientMonid does not have a _repr_; add this and unify _repr_ of GenericTermMonoid.

Change History (13)

comment:1 Changed 10 months ago by dkrenn

  • Branch set to u/dkrenn/term-monoids-unify-repr

comment:2 Changed 10 months ago by dkrenn

  • Authors set to Daniel Krenn
  • Commit set to a0b6a1e235cb112b5a00b810cfd9cd0c65ea2521
  • Status changed from new to needs_review

New commits:

9061510add _repr_ for TermWithCoefficientMonoid
a0b6a1eupdate and unify GenericTermMonoid _repr_

comment:3 Changed 10 months ago by dkrenn

  • Cc cheuberg behackl gh-thhagelmayer added

comment:4 follow-up: Changed 10 months ago by behackl

  • 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 X-Monoid ..., 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 OTerm-Monoid, instead of the current O-Term Monoid.

Other than that, these changes LGTM.

comment:5 in reply to: ↑ 4 Changed 10 months ago by dkrenn

Replying to behackl:

There is one thing I'd briefly like to discuss: The representation strings introduced and modified here start with X-Monoid ..., 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 OTerm-Monoid, instead of the current O-Term Monoid.

I am against this change. I think there are two ways to write the monoids, either X-Monoid where X is a class name or a more verbose name if possible, which is the case for O-Term Monoid (and also for B-Term 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 follow-up: Changed 10 months ago by 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 as-is. Your suggestion with the removal of the dashes before Monoid is fine for me (i.e., TermWithCoefficient Monoid and O-Term Monoid).

comment:7 Changed 10 months ago by git

  • Commit changed from a0b6a1e235cb112b5a00b810cfd9cd0c65ea2521 to 2f9c6b6eeb3f51ee0b919b8ab6da2edd67fae3ec

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

2f9c6b6Trac #32214: unify dash in monoid repr

comment:8 in reply to: ↑ 6 Changed 10 months ago by dkrenn

  • 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 as-is. Your suggestion with the removal of the dashes before Monoid is fine for me (i.e., TermWithCoefficient Monoid and O-Term Monoid).

Ok. Changed.

comment:9 Changed 10 months ago by behackl

  • Branch changed from u/dkrenn/term-monoids-unify-repr to u/behackl/term-monoids-unify-repr

comment:10 Changed 10 months ago by behackl

  • Commit changed from 2f9c6b6eeb3f51ee0b919b8ab6da2edd67fae3ec to f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57
  • Reviewers set to Benjamin Hackl

Thanks! I've pushed two minor reviewer commits, feel free to cross-review 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:

5ef7eceadapted TermWithCoefficientMonoid._repr_ summary
f76ffc6simplified doctest via DefaultTermMonoidFactory

comment:11 Changed 10 months ago by dkrenn

  • Status changed from needs_review to positive_review

comment:12 Changed 9 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:13 Changed 9 months ago by vbraun

  • Branch changed from u/behackl/term-monoids-unify-repr to f76ffc6c05c1f85d2cc6dd5916af1d52a5863e57
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.