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 differIf 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
WarrenWeckesser
left a comment
There was a problem hiding this comment.
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.
| 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.
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 <[email protected]>
Match new definition and add versionadded.
Co-authored-by: Warren Weckesser <[email protected]>
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
symbolattribute to the polynomials from thenp.polynomialpackage to allow the user to control/modify the symbol used to represent the independent variable for a polynomial expression. This attribute corresponds to thevariableattribute of thepoly1dclass from the oldnp.lib.polynomialmodule.Marked as draft for now as it depends on #15666 - all
_str*and_repr*methods ofABCPolyBaseand derived classes would need to be modified (and tested) to support this change.