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

ENH: Add support for symbol to polynomial package #16154

Merged
merged 37 commits into from Jun 1, 2022

Conversation

rossbar
Copy link
Contributor

@rossbar rossbar commented May 4, 2020

Adds a symbol attribute to the polynomials from the np.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 the variable attribute of the poly1d class from the old np.lib.polynomial module.

Marked as draft for now as it depends on #15666 - all _str* and _repr* methods of ABCPolyBase and derived classes would need to be modified (and tested) to support this change.

@eric-wieser
Copy link
Member

to allow the user to control/modify the symbol used to represent the independent variable for a polynomial expression.

Just to play devil's advocate - is the a reason we actually need to store a symbol, or is this just about printing? '{:ascii(y)}'.format(p) would be an alternative way to request a specific symbol when displaying, although I suppose that doesn't help _repr_latex_.

@rossbar
Copy link
Contributor Author

rossbar commented May 8, 2020

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!

@rossbar rossbar removed the 25 - WIP label Jun 13, 2020
@rossbar rossbar marked this pull request as ready for review June 13, 2020 01:25
@rossbar rossbar changed the title WIP,ENH: Add support for symbol to polynomial package ENH: Add support for symbol to polynomial package Jun 29, 2020
@rossbar
Copy link
Contributor Author

rossbar commented Sep 22, 2020

I had forgotten to include the symbol in _repr_latex_ - thanks @eric-wieser for the reminder.

This PR adds support for different symbols in the str/repr of instances of the polynomial convenience classes. The poly1d class from the polynomial module has a similar feature, though there it is called variable instead of symbol. I am happy to change the proposed attribute name to match the precedent set by poly1d, though I think symbol is more descriptive.

@seberg seberg added the triage review Issue/PR to be discussed at the next triage meeting label Sep 23, 2020
@seberg seberg removed the triage review Issue/PR to be discussed at the next triage meeting label Oct 7, 2020
@stefanv
Copy link
Contributor

stefanv commented Oct 15, 2020

What's the status on this PR? It looks like comments have been addressed.

Base automatically changed from master to main March 4, 2021 02:04
@rossbar
Copy link
Contributor Author

rossbar commented Apr 28, 2022

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 symbol attribute to the polynomial classes that allows a user to select the symbol representing the indeterminate. This also reinforces the fact that the polynomial classes are 1D by validating operations between polynomial expressions to ensure that the symbols are the same. The main motivation for this work is feature parity with the poly1d class, which has a variable attribute.

@stefanv
Copy link
Contributor

stefanv commented May 26, 2022

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.

@WarrenWeckesser
Copy link
Member

The symbol is propagated correctly when polynomials are composed, e.g.

In [90]: p = np.polynomial.Polynomial([3, 2, 1], symbol="t")

In [91]: q = np.polynomial.Polynomial([5, 1, 0, -1], symbol="λ_1")

In [92]: r = p(q)

In [93]: r.symbol
Out[93]: 'λ_1'

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.

Copy link
Member

@WarrenWeckesser WarrenWeckesser left a 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")
Copy link
Contributor

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).

@seberg seberg added the 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. label Jun 1, 2022
rossbar and others added 24 commits June 1, 2022 13:27
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>
@seberg
Copy link
Member

seberg commented Jun 1, 2022

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 :).

@seberg seberg merged commit b84a53d into numpy:main Jun 1, 2022
BvB93 added a commit to BvB93/numpy that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 - Enhancement 54 - Needs decision 62 - Python API Changes or additions to the Python API. Mailing list should usually be notified. component: numpy.polynomial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants