Opened 8 years ago

Closed 8 years ago

#14120 closed enhancement (fixed)

Add constant_coefficient method for Laurent polynomials

Reported by: tscrim Owned by: tscrim
Priority: major Milestone: sage-5.8
Component: algebra Keywords: days45
Cc: Merged in: sage-5.8.beta2
Authors: Travis Scrimshaw Reviewers: Kannappan Sampath
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

There is no (easy) way to obtain the coefficient on the constant term for an element in the Laurent polynomial ring. This just adds a method constant_coefficient().

Attachments (1)

trac_14120-constant_coeff_laurent_poly-ts.patch (7.3 KB) - added by tscrim 8 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 8 years ago by tscrim

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by tscrim

Fixed doctests and added a general __getitem__() to retrieve the coefficient by exponent value:

sage: P.<x,y> = LaurentPolynomialRing(QQ)
sage: f = (y^2 - x^9 - 7*x*y^3 + 5*x*y)*x^-3; f
-x^6 - 7*x^-2*y^3 + 5*x^-2*y + x^-3*y^2
sage: f[-2,3]   
-7
sage: f.coefficient(x^-2*y^3)
-7

comment:3 follow-up: Changed 8 years ago by knsam

  • Reviewers set to Kannappan Sampath
  • Status changed from needs_review to needs_work

I thought, I'd review this ticket.

The docstring claims that, if the number of inputs is less than the number of variables, all the remaining[trailing] variables would have their exponent set to zero. But, for some reason, the implementation (and hence the examples) raises a TypeError.

IMHO, it would be nice to implement the behaviour suggested by the docstring. (of course, the other case, where no. of variables <= no. of inputs is being handled correctly.)

And, could we also please document that multivariate laurent polynomials are not iterable, as implemented, by adding an example, before we get complaints "like" this.

Building documentation, there is a warning, which is fairly easy to fix: I think the complaint is about the method __call__

<autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.

Finally, that we are changing this file, one might as well go ahead and clean up all the codifications: self is almost never codified, for instance and such documentation stuff...

The already extant method cofficient() would benefit from adding r: r""" just so that the \frac with backslash is picked up properly by Sphinx. There is a misspelt word in the Note that is already there too. (constnat...)


Regards, ~KnS

Last edited 8 years ago by knsam (previous) (diff)

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

  • Status changed from needs_work to needs_review

First off, thank you for doing the review.

Replying to knsam:

The docstring claims that, if the number of inputs is less than the number of variables, all the remaining[trailing] variables would have their exponent set to zero. But, for some reason, the implementation (and hence the examples) raises a TypeError.

IMHO, it would be nice to implement the behaviour suggested by the docstring. (of course, the other case, where no. of variables <= no. of inputs is being handled correctly.)

I made it consistent with other multivariate polynomial rings, however I forgot to correct the doc. This is done. I've also added in two more methods for consistancy and changed the output of the iterator.

And, could we also please document that multivariate laurent polynomials are not iterable, as implemented, by adding an example, before we get complaints "like" this.

This does not make sense to me. They are iterable with this patch.

Building documentation, there is a warning, which is fairly easy to fix: I think the complaint is about the method __call__

<autodoc>:0: WARNING: Block quote ends without a blank line; unexpected unindent.

I do not get this with the revised patch.

Finally, that we are changing this file, one might as well go ahead and clean up all the codifications: self is almost never codified, for instance and such documentation stuff...

You are welcome to do so on another ticket. That is outside of the purview of this one. I did it in the methods this patch touches.

The already extant method cofficient() would benefit from adding r: r""" just so that the \frac with backslash is picked up properly by Sphinx. There is a misspelt word in the Note that is already there too. (constnat...)

Fixed.

Best, Travis

comment:5 Changed 8 years ago by knsam

  • Status changed from needs_review to positive_review

And, could we also please document that multivariate laurent polynomials are not iterable, as implemented, by adding an example, before we get complaints "like" this.

This does not make sense to me. They are iterable with this patch.

I meant more along the following lines: you cannot iterate through using slice notation:

 sage: P.<x,y,z> = LaurentPolynomialRing(QQ)
 sage: f = (y^2 - x^9 - 7*x*y^3 + 5*x*y)*x^-3 + x*z; f
 -x^6 + x*z - 7*x^-2*y^3 + 5*x^-2*y + x^-3*y^2
 sage: f[6,0,0]
 -1
 sage:f[5:6]
 [snip]
 TypeError: Multivariate Laurent polynomials are not iterable

But, well, this does not even make sense, given the current implementation. So, I'll set this to positive review.

~KnS

comment:6 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.8.beta2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.