BUG: Fix scipy incompatibility with cleanup to poly1d

[eric-wieser]

Scipy was relying on some of the broken things that #8762 fixed

[charris]

I think this should work. Might make a note in __init__ that assigning to self._coeffs creates and initializes self.coeffs.

[charris]

Maybe the easiest thing to do all round is to make the attributes writable.

[eric-wieser]

Might make a note in init that assigning to self._coeffs creates and initializes self.coeffs

I'd argue not - what _coeffs does is an implementation detail.

Maybe the easiest thing to do all round is to make the attributes writable.

If you measure easiness by lines changed, that's actually less easy!

[charris]

In the original poly1d it would only have required removing {get,set}attr plus the equality of the attribute aliases.

[eric-wieser]

Yes, that's true. Although making order writeable doesn't make any sense, so that should remain a property as it is in the new poly1d

[seberg]

If there is an easy way to fix the "broken scipy", we could also put a deprecation warning somewhere probably.

[eric-wieser]

@seberg: I've already submitted the easy fix to "broken scipy" in scipy/scipy#7184

[charris]

I'd argue not - what _coeffs does is an implementation detail.

Which is why it needs a comment in the code...

[eric-wieser]

By that, I mean its an implementation detail of _coeffs not of poly1d. Perhaps the comments I did add aren't descriptive enough, but I think they're in the right place

[charris]

The problem is that one needs to read through a lot of garbage code and implementation details to figure out how coeffs gets into the dictionary. If that wasn't a problem in scipy no one would care, but given the implementation of orthopoly1d and the problems it has caused, it would be good for it to be clear how it got fixed for future reference.

[charris]

So at line 1098, something like

# This also initializes self.coeffs for backwards compatibility
# with scipy.special.orthogonal.orthopoly1d.
self._coeffs = c_or_r

[mhvk]

This is really ugly, and now you end up with a coeff attribute that can actually be modified, while the problem is that scipy is relying on self.__dict__ containing 'coeff' and modifying that. Since that is so, how about (inspired by lazyproperty):

def get_coeffs(self):
    return self.__dict__['coeffs']

def set_coeff(self, coeffs):
    if 'coeffs' in self.__dict__:
        raise AttributeError(...)
    self.__dict__['coeffs'] = coeffs

coeffs = property(get_coeffs, set_coeffs)

(note that the property itself gets store in the class's __dict__ and not in an instance's __dict__). Obviously, it would then still need a note that it should become a regular property eventually, and you need to change __init__ to set self.coeff rather than self._coeff.

[charris]

@mhvk I like that. Bit tricksy -- I would not have understood it without the explanation -- but definitely cleaner.

[eric-wieser]

@mhvk: Huh, wasn't away that while __dict__ takes priority over property.fget, property.fset takes priority over __dict__.

I still think that we want poly1d to use _coeffs internally, because that's what we want the api to look like. So perhaps:

# our internal _coeffs property need to be backed by __dict__['coeffs'] for scipy to work
@property
def _coeffs(self):
    return self.__dict__['coeffs']
@_coeffs.setter
def _coeffs(self, coeffs):
    self.__dict__['coeffs'] = coeffs

# expose a readonly copy of the coefficients publicly
# note that this does not return a copy for .coeffs, as the above workaround overwrites
# this getter. It does not, however, affect the behaviour for .c, .coef, or .coefficients
@property
def coeffs(self):
    return self._coeffs.copy()
c = coef = coefficients = coeffs

[charris]

Both the proposed new versions work, although only if poly1d is derived from object, as it is. @eric-wieser I'll leave the choice up to you, but we need to get this in.

[eric-wieser]

@charris: Ok, updated with my suggestion.

Is the second commit reasonable, given that it makes .coef and .coefficients copies of data, but .coeffs is the original (because making it a copy is not possible)?

[charris]

I think it is worth a shot. We will see if anyone complains in the 1.13.0, aka FU, release.

[charris]

I think that p.coeffs is still a copy. Apparently __getattribute__ assigns priority in way that makes this all work. See https://docs.python.org/2/howto/descriptor.html.

[charris]

Thanks Eric.

[charris]

If you look at the class dict, poly1d.__dict__, I think you will see that c, coef, coefficients, coeffs, are all the same property, and properties seem to have precedence.

[eric-wieser]

@charris: I really don't think that is the case, else @mhvk's remark about this being derived from @lazyproperty doesn't apply - that relies on being able to overwrite the getter with the value.

And if it is the case, then I need to correct the comment I made in the code saying that it wasn't the case ...

[eric-wieser]

Well, it turns out you're right! I guess I need to rethink my understanding of the property model, and make a PR to fix this PR fixing that PR...

[eric-wieser]

This comment is false

[mhvk]

Sorry to have been out of the loop, but indeed the second sentence is incorrect.

[eric-wieser]

Fixed in #8807