Opened 4 years ago

Closed 4 years ago

#19981 closed enhancement (fixed)

is_exact for asymptotic ring

Reported by: behackl Owned by:
Priority: major Milestone: sage-7.1
Component: asymptotic expansions Keywords:
Cc: cheuberg, dkrenn Merged in:
Authors: Benjamin Hackl Reviewers: Daniel Krenn, Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: da853f8 (Commits) Commit: da853f87105d7e8ec4883141e1a98911cb91900c
Dependencies: Stopgaps:

Description (last modified by behackl)

This ticket shall implement a method that returns wheter all terms of some asymptotic expansion are exact terms.

Change History (17)

comment:1 follow-up: Changed 4 years ago by behackl

  • Branch set to u/behackl/asy/misc-improvements
  • Commit set to 550e7178b91b312f1513220f05045efb62dd5eb4

New commits:

cc0137dimplement is_exact
550e717implement _latex_

comment:2 follow-up: Changed 4 years ago by behackl

Note: this is on top of 7.1.beta1.

Any more suggestions for practical improvements?

comment:3 in reply to: ↑ 2 ; follow-up: Changed 4 years ago by cheuberg

Replying to behackl:

Note: this is on top of 7.1.beta1.

Any more suggestions for practical improvements?

no; they could go into a separate ticket anyway.

comment:4 in reply to: ↑ 3 Changed 4 years ago by dkrenn

Replying to cheuberg:

they could go into a separate ticket anyway.

+1 for separate tickets.

comment:5 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by dkrenn

I had a quick look at the code:

cc0137dimplement is_exact

IMHO inefficient; has_same_summands should do better, since no new mutable posets have to be created.

550e717implement _latex_

SR uses the commutativity of the multiplication sometimes, so LaTeX-output might differ from repr-output. I suggest to implement _latex_ for terms and growth groups as they know what to do.

comment:6 in reply to: ↑ 5 Changed 4 years ago by behackl

Replying to dkrenn:

I had a quick look at the code:

cc0137dimplement is_exact

IMHO inefficient; has_same_summands should do better, since no new mutable posets have to be created.

I'd even use _has_same_summands_, the parent is certainly the same so this is even more efficient. Will change later.

550e717implement _latex_

SR uses the commutativity of the multiplication sometimes, so LaTeX-output might differ from repr-output. I suggest to implement _latex_ for terms and growth groups as they know what to do.

That was/is my original plan, and this is also the reason why I didn't set this to needs_review. ;-) However, I wanted to have nice (growth-ordered) latex-output quickly in order to check the output of the singularity analysis generator; and this was the simplest implementation I could think of. The code is just dumped here.

I'll also split this ticket into two.

comment:7 Changed 4 years ago by behackl

  • Branch changed from u/behackl/asy/misc-improvements to u/behackl/asy/is_exact
  • Commit changed from 550e7178b91b312f1513220f05045efb62dd5eb4 to 26639e22157016fa1410ac8e14affd391779b6b9
  • Description modified (diff)
  • Status changed from new to needs_review
  • Summary changed from asymptotic expansions: minor improvements to is_exact for asymptotic ring

New commits:

09a3db2implement is_exact
26639e2improve performance of is_exact

comment:8 Changed 4 years ago by dkrenn

  • Reviewers set to Daniel Krenn

Typo: "Nothin.", but you could remove the INPUT-block completely. Otherwise looks good. Still have to wait until I have a working 7.1.beta1.

PS: Author missing

comment:9 follow-up: Changed 4 years ago by cheuberg

  • Reviewers changed from Daniel Krenn to Daniel Krenn, Clemens Heuberger

Apart from that: while the routine is hardly efficiency critical, it is somewhat inefficient to construct exact_part and then comparing with the original just to check whether all summands are ExactTerms. If this is to ensure that future extensions of the term monoids won't introduce inconsistencies, then there could be a helper method is_exact.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 4 years ago by dkrenn

Replying to cheuberg:

Apart from that: while the routine is hardly efficiency critical, it is somewhat inefficient to construct exact_part and then comparing with the original just to check whether all summands are ExactTerms. If this is to ensure that future extensions of the term monoids won't introduce inconsistencies, then there could be a helper method is_exact.

True. Maybe something like all(T.is_exact() for T in self.summands)...

comment:11 Changed 4 years ago by dkrenn

  • Status changed from needs_review to needs_work

comment:12 in reply to: ↑ 10 Changed 4 years ago by cheuberg

Replying to dkrenn:

True. Maybe something like all(T.is_exact() for T in self.summands)...

That's what I meant.

comment:13 Changed 4 years ago by behackl

I'm not a very big fan of having a method is_exact for terms, where this would just return isinstance(self, ExactTerm). I think that this would just add an unneccessary layer of function calls.

Are you against something like all(isinstance(T, ExactTerm) for T in self.summands)?

comment:14 Changed 4 years ago by git

  • Commit changed from 26639e22157016fa1410ac8e14affd391779b6b9 to da853f87105d7e8ec4883141e1a98911cb91900c

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

9042dabimplement is_exact for terms
f5bdb8freplace isinstance(..., ExactTerm) with is_exact
f65bc42remove INPUT in docstring
da853f8implement expansion.is_exact() by using term.is_exact()

comment:15 Changed 4 years ago by behackl

  • Authors set to Benjamin Hackl
  • Status changed from needs_work to needs_review

I've changed my mind: to keep our style of programming consistent, I have implemented an is_exact-method for terms and replaced all isinstance(term, ExactTerm)-statements by term.is_exact().

Back to needs_review.

comment:16 Changed 4 years ago by dkrenn

  • Status changed from needs_review to positive_review

Seems to be fine now.

comment:17 Changed 4 years ago by vbraun

  • Branch changed from u/behackl/asy/is_exact to da853f87105d7e8ec4883141e1a98911cb91900c
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.