ENH: Enforce constraints on polynomial attrs window and domain

[rossbar]

Related to #16059

The changes here represent a potential solution to the mutability of two (of the three) defining attributes of polynomials: the domain and window attributes.

The mutability of these attrs leads to some undesirable behavior. For example, any array-like with len == 2 is currently allowed, but the __repr__ expects that they must be arrays. Thus setting the domain or window with a list will cause problems:

>>> p = np.polynomial.Polynomial([1, 2, 3])
>>> p
Polynomial([1., 2., 3.], domain=[-1,  1], window=[-1,  1])
>>> p.domain = [-2, 2]
>>> p
Polynomial([1., 2., 3.], domain=, window=[-1,  1])

Note that __init__ does input validation and converts array-like to the expected format:

>>> p = np.polynomial.Polynomial([1, 2, 3], domain="foo")   # invalid
ValueError: Coefficient arrays have no common type
>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-2, 2])
>>> p
Polynomial([1., 2., 3.], domain=[-2.,  2.], window=[-1,  1])

This PR reorganizes the classes in the polynomial module to add the same input validation used by __init__ to a general setter for the domain and window attributes.

[eric-wieser]

Could consider:

Suggested change

	[domain] = pu.as_series([domain], trim=False)
	if len(domain) != 2:
	raise ValueError("Domain has wrong number of elements.")
	self._domain = domain
	if domain is None:
	# remove the instance-specific value to restore the default
	try:
	del self._domain
	except KeyError:
	pass
	return
	[domain] = pu.as_series([domain], trim=False)
	if len(domain) != 2:
	raise ValueError("Domain has wrong number of elements.")
	self._domain = domain

[eric-wieser]

or

Suggested change

	[domain] = pu.as_series([domain], trim=False)
	if len(domain) != 2:
	raise ValueError("Domain has wrong number of elements.")
	self._domain = domain
	if domain is None:
	self._domain = type(self)._domain
	return
	[domain] = pu.as_series([domain], trim=False)
	if len(domain) != 2:
	raise ValueError("Domain has wrong number of elements.")
	self._domain = domain

[rossbar]

As I understand it, the main thing that this code does is allow None as a valid value for the domain setter. By example:

Current behavior of NumPy

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
>>> p.domain

Behavior with this PR

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
ValueError: Domain has wrong number of elements.
>>> p.domain
array([-10.,  10.])

Behavior with proposed change

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p 
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
>>> p.domain
array([-1., 1.])

Is this correct and the change that you intended? IMO, I prefer the version that errors rather than giving None a special meaning that falls back to the default value for the class. This could be achieved via p.domain = np.polynomial.Polynomial.domain.

[eric-wieser]

A little alarming that these are mutable.

[rossbar]

Do you mean that the original (red) is alarming or the proposed change? I had figured the prepended-_-means-"private" convention was enough to discourage any user from changing the default class variables.

[rossbar]

Thanks for taking a look at this!

I should mention that my main goal in this PR was to introduce some mechanism by which the domain and window could be made immutable both at the class and instance level, without breaking backwards compatibility.

I introduced abstract setters (with default implementations) that basically take the constraints applied to the inputs in the constructor and apply them to any time a user would try to change the domain or window attributes on-the-fly. Note that instead the setters could be used to raise a future or deprecation warning instead if it were decided that the domain and window should be made immutable (forcing the user to use convert() or create a new instance to change these attrs).

[rossbar]

As I understand it, the main thing that this code does is allow None as a valid value for the domain setter. By example:

Current behavior of NumPy

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
>>> p.domain

Behavior with this PR

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
ValueError: Domain has wrong number of elements.
>>> p.domain
array([-10.,  10.])

Behavior with proposed change

>>> p = np.polynomial.Polynomial([1, 2, 3], domain=[-10, 10])
>>> p 
Polynomial([1., 2., 3.], domain=[-10.,  10.], window=[-1,  1])
>>> p.domain = None
>>> p.domain
array([-1., 1.])

Is this correct and the change that you intended? IMO, I prefer the version that errors rather than giving None a special meaning that falls back to the default value for the class. This could be achieved via p.domain = np.polynomial.Polynomial.domain.

[rossbar]

Do you mean that the original (red) is alarming or the proposed change? I had figured the prepended-_-means-"private" convention was enough to discourage any user from changing the default class variables.

[rossbar]

I rebased to fix up the merge conflicts. This PR needs a decision on whether or not the change is worthwhile.

A quick recap: this PR adds some input-checking to the setting of the window and domain attributes on convenience class instances. This fixes problems like the following:

>>> import numpy.polynomial as npp
>>> p = npp.Polynomial([1, 2, 3])
>>> p
Polynomial([1., 2., 3.], domain=[-1,  1], window=[-1,  1])

# The domain and window can be set to arbitrary objects, leading to 
# strange behavior in e.g. the repr
>>> p.window = [-2, 3]   # a perfectly valid window
>>> p
Polynomial([1., 2., 3.], domain=[-1,  1], window=)

# or strange errors during computation
>>> p.window = "2, 3"   # Should be disallowed, but isn't
>>> p(10)
Traceback (most recent call last)
   ...
TypeError: unsupported operand type(s) for -: 'str' and 'str'

This PR proposes a solution that still allows window and domain to be modified on the instance but requires the object to be a length-2 sequence, and fails with a descriptive message when this is not the case.

[rossbar]

I'll go ahead and close this as it's not a critical feature. If ever we want to enforce the window/domain constraints I think this is a decent way to do it.