Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coefficient of an AsymptoticExpansion #22340

Closed
cheuberg opened this issue Feb 10, 2017 · 14 comments
Closed

Coefficient of an AsymptoticExpansion #22340

cheuberg opened this issue Feb 10, 2017 · 14 comments

Comments

@cheuberg
Copy link
Contributor

The following would be convenient:

sage: R.<m, n> = AsymptoticRing("m^QQ*n^QQ", ZZ)
sage: ae = 1 + 42/n + 2/n/m + O(n^-2)
sage: ae.monomial_coefficient(1/n)
42

The choice of the method name monomial_coefficient is motivated by the homonymous method of multivariate polynomial rings.

CC: @dkrenn @behackl

Component: asymptotic expansions

Author: Clemens Heuberger

Branch/Commit: f04154b

Reviewer: Daniel Krenn

Issue created by migration from https://trac.sagemath.org/ticket/22340

@cheuberg
Copy link
Contributor Author

@cheuberg
Copy link
Contributor Author

Author: Clemens Heuberger

@cheuberg
Copy link
Contributor Author

New commits:

6242878Trac #22340: Coefficient of an AsymptoticExpansion

@cheuberg
Copy link
Contributor Author

Commit: 6242878

@dkrenn
Copy link
Contributor

dkrenn commented Feb 10, 2017

comment:3

Looks good; I only have very small things to discuss:

+        - ``monomial`` -- a monomial element which can be converted
+          into the parent of this element.

What about rephrasing this to the following, so that the technical term "parent" is avoided

a monomial element which can be converted into the asymptotic ring of this element
  1. Remove period after "element." above (convention in SageMath; not used in the asymptotic_ring file much...).

+            raise ValueError("non-exact monomial")
+            raise ValueError("not a monomial")

Within the asymptotic ring module usually error messages include the element (in the above, the monimial), i.e.

raise ValueError('non-exact monimal {}'.format(monomial))
raise ValueError('{} not a monomial'.format(monomial))

(Maybe inserting "is" in the latter, but not sure.)

@dkrenn
Copy link
Contributor

dkrenn commented Feb 10, 2017

Reviewer: Daniel Krenn

@dkrenn
Copy link
Contributor

dkrenn commented Feb 10, 2017

comment:4
  1. The method in the multivariate polynomial ring returns 0 if the monomial is not in the polynomial. Here a KeyError will be raised. If there is no reason against 0, I think the behavior should coincide.

@dkrenn
Copy link
Contributor

dkrenn commented Feb 10, 2017

comment:5
  1. FYI, the following (undocumented) feature of a polynomial is present:
sage: P.<x,y> = ZZ[]
sage: a=(1+2*x*y+3*x^2)
sage: a[x*y]
2
sage: a[x^2]
3

So maybe __getitem__ should have this behavior as well...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2017

Changed commit from 6242878 to f04154b

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2017

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

b6f670bTrac #22340.1+2 rephrase input documentation
621a006Trac #22340.3: Mention monomial in error message
f04154bTrac #22340.4: return 0 when appropriate

@cheuberg
Copy link
Contributor Author

comment:7

1.-4. done
5. I am not convinced: If this is not a documented and tested behaviour of polynomial rings, I am slightly hesitant to introduce this here. I am not against the concept, but IMHO it should be done in the polynomial ring first (and not on this ticket).

@dkrenn
Copy link
Contributor

dkrenn commented Feb 11, 2017

comment:8

Replying to @cheuberg:

1.-4. done

Thank you.

  1. I am not convinced: If this is not a documented and tested behaviour of polynomial rings, I am slightly hesitant to introduce this here. I am not against the concept, but IMHO it should be done in the polynomial ring first (and not on this ticket).

Fine for me not to do this now.

Patch LGTM; positive_review modulo non-finished patchbot

@dkrenn
Copy link
Contributor

dkrenn commented Feb 13, 2017

comment:9

Two patchbot tested this ticket now; each timed out on a single, but different doctest, and these tests are not related to this ticket. Thus I set it to positive review

@vbraun
Copy link
Member

vbraun commented Feb 16, 2017

Changed branch from u/cheuberg/coefficient-asymptotic-expansion to f04154b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants