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
ENH: Add support for symbol to polynomial package #16154
Conversation
Just to play devil's advocate - is the a reason we actually need to store a symbol, or is this just about printing? |
That's a good point - printing with different symbols is definitely the primary motivation, but it also reinforces the fact that the polynomial convenience classes are intended to represent 1-D polynomials. The majority of the changes in the PR have to do with disallowing operations between polynomials with different symbols, e.g. >>> p1 = np.polynomial.Polynomial([1, 2, 3], symbol='x')
>>> p2 = np.polynomial.Polynomial([2, 3, 4], symbol='y')
>>> p1 + p2
ValueError: Polynomial symbols differ If this secondary component is not important (or maybe even undesirable) and it truly is only the printing that is important, then exploring solutions along the lines you've suggested would certainly be worthwhile. IMO printing is the main thing, but I don't have a lot of the big-picture context surrounding the polynomial module. I will try to bring up this point at the next community meeting! |
77fffd5
to
a15ae0b
Compare
ce0fd3d
to
0043fcc
Compare
I had forgotten to include the symbol in This PR adds support for different symbols in the str/repr of instances of the polynomial convenience classes. The |
What's the status on this PR? It looks like comments have been addressed. |
92f254c
to
440ad13
Compare
I've gone ahead and updated this one as it's been a while without any activity. Just a quick refresher on this PR: it adds a |
This was a request from someone who teaches using the polynomial module. The PR solves their problem, and with Warren's suggestion included feels quite safe. So, I'm +1. |
The symbol is propagated correctly when polynomials are composed, e.g.
The test suite looks thorough, but I don't see a test for composition. Maybe add one? Other than that and my other docstring suggestion, this looks good to me. |
0155a75
to
b4124d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rossbar, looks good.
If/when this is merged, the commits should be squashed. Don't bother trying to preserve the attribution for my commit; it was trivial and changed later anyway.
@@ -284,10 +296,12 @@ class as self with identical domain and window. If so, | |||
raise TypeError("Domains differ") | |||
elif not np.all(self.window == other.window): | |||
raise TypeError("Windows differ") | |||
elif self.symbol != other.symbol: | |||
raise ValueError("Polynomial symbols differ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should here be some guidance on renaming the symbol? Something like: to change a polynomial's symbol, use p.symbol = 't'
(or whatever the right way is).
Parametrized binary ops tests for poly symbol to test both polynomials and arrays. Created new class to encapsulate unary operators (pow, neg, etc)
Added tests for class methods that create poly instance. Make 'symbol' a RO property of ABCPolyBase. Updated routines.polynomials.classes for refguide_check
* Add tests for symbol in TestLatexRepr
Raise informative exceptions when the symbol input is invalid. Co-authored-by: Warren Weckesser <warren.weckesser@gmail.com>
Match new definition and add versionadded.
Co-authored-by: Warren Weckesser <warren.weckesser@gmail.com>
b4124d1
to
8b6dda5
Compare
8b6dda5
to
da2fae3
Compare
Might be that the release note could be snipped down a bit, but thanks for adding it! Thanks Ross, Warren, and Stéfan for pushing this forward, let me do the lazy part and press the button :). |
Adds a
symbol
attribute to the polynomials from thenp.polynomial
package to allow the user to control/modify the symbol used to represent the independent variable for a polynomial expression. This attribute corresponds to thevariable
attribute of thepoly1d
class from the oldnp.lib.polynomial
module.Marked as draft for now as it depends on #15666 - all
_str*
and_repr*
methods ofABCPolyBase
and derived classes would need to be modified (and tested) to support this change.