[Numpy-discussion] Correct error of invalid axis arguments.
Hi All, At the moment there are two error types raised when invalid axis arguments are encountered: IndexError and ValueError. I prefer ValueError for arguments, IndexError seems more appropriate when the bad axis value is used as an index. In any case, having mixed error types is inconvenient, but also inconvenient to change. Should we worry about that? If so, what should the error be? Note that some of the mixup arises because the axis values are not checked before use, in which case IndexError is raised. Chuck ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] Correct error of invalid axis arguments.
On Mo, 2016-09-05 at 11:54 -0600, Charles R Harris wrote: > Hi All, > > At the moment there are two error types raised when invalid axis > arguments are encountered: IndexError and ValueError. I prefer > ValueError for arguments, IndexError seems more appropriate when the > bad axis value is used as an index. In any case, having mixed error > types is inconvenient, but also inconvenient to change. Should we > worry about that? If so, what should the error be? Note that some of > the mixup arises because the axis values are not checked before use, > in which case IndexError is raised. > I am not too bothered about it myself, but yes, it is a bit annoying. My gut feeling on it would be to not worry about it much, unless we implement some more general input validator for the python side (which possibly could do even more like validate/convert input arrays all in one go). Putting explicit guards to every single python side function is of course possible too, but I am not quite convinced its worth the trouble. - Sebastian > Chuck > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
Hi Sebastian, Indeed, having the scalar pass through `__array_wrap__` would have been useful (_finalize__ is too late, since one cannot change the class any more, just set attributes). But that is water under the bridge, since we're stuck with people not expecting that. I think the slightly larger question, but one somewhat orthogonal to your suggestion of a new dundermethod, is whether one cannot avoid more such methods by the new indexing routines returning array scalars instead of regular ones. Obviously, though, this has larger scope, as it might be part of the merging of the now partially separate code paths for scalar and array arithmetic, etc. All the best, Marten ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Mo, 2016-09-05 at 14:54 -0400, Marten van Kerkwijk wrote: > Hi Sebastian, > > Indeed, having the scalar pass through `__array_wrap__` would have > been useful (_finalize__ is too late, since one cannot change the > class any more, just set attributes). But that is water under the > bridge, since we're stuck with people not expecting that. > > I think the slightly larger question, but one somewhat orthogonal to > your suggestion of a new dundermethod, is whether one cannot avoid > more such methods by the new indexing routines returning array > scalars > instead of regular ones. > > Obviously, though, this has larger scope, as it might be part of the > merging of the now partially separate code paths for scalar and array > arithmetic, etc. Thanks for the input. I am not quite sure about all of the things. Calling array wrap for the scalar returns does not sound like a problem (it would also effect other code paths). Calling it only for the new methods creates a bit of branching, but is not a big deal. Would it help you though? You could avoid implementing all the new indexing methods for many/most subclasses, but how do you tell numpy that you are supporting them? Right now I thought it would make sense to give an error if you try `subclass.vindex[...]` but the subclass has `__getitem__` implemented (and not overwritten vindex). The dundermethod gives a way to tell numpy: you know what to do. For the sake of masked arrays it is also convenient (you can use the indexer also on the mask), but masked arrays are rather special. It would be interesting if there are more complex subclasses out there, which implement `__getitem__` or `__setitem__`. Maybe all we need is some new trick for the scalars and most subclasses can just remove their `__getitem__` methods - Sebastian > > All the best, > > Marten > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion > signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
Hi Sebastian, It would seem to me that any subclass has to keep up to date with new features in ndarray, and while I think ndarray has a responsibility not to break backward compatibility, I do not think it has to protect against new features possibly not working as expected in subclasses. In particular, I think it is overly complicated (and an unnecessary maintenance burden) to error out if a subclass has `__getitem__` overwritten, but not `oindex`. For somewhat similar reasons, I'm not too keen on a new `__numpy_getitem__` method; I realise it might reduce complexity for some ndarray subclasses eventually, but it also is an additional maintenance burden. If you really think it is useful, I think it might be more helpful to define a new mixin class which provides a version of all indexing methods that just call `__numpy_getitem__` if that is provided by the class that uses the mixin. I would *not* put it in `ndarray` proper. Indeed, the above might even be handier for subclasses, since they can choose, if they wish, to implement a similar mixin for older numpy versions, so that all the numpy version stuff can be moved to a single location. (E.g., I can imagine doing the same for `__numpy_ufunc__`.) Overall, my sense would be to keep your PR to just implementing the various new index methods (which are great -- I still don't really like the names, but sadly don't have better suggestions...). But it might be good if others pipe in here too, in particular those maintaining ndarray subclasses! All the best, Marten ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
Actually, on those names: an alternative to your proposal would be to introduce only one new method which can do all types of indexing, depending on a keyword argument, i.e., something like ``` def getitem(self, item, mode='outer'): ... ``` -- Marten ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Monday, September 5, 2016, Marten van Kerkwijk wrote: > Hi Sebastian, > > It would seem to me that any subclass has to keep up to date with new > features in ndarray, and while I think ndarray has a responsibility > not to break backward compatibility, I do not think it has to protect > against new features possibly not working as expected in subclasses. > In particular, I think it is overly complicated (and an unnecessary > maintenance burden) to error out if a subclass has `__getitem__` > overwritten, but not `oindex`. > > For somewhat similar reasons, I'm not too keen on a new > `__numpy_getitem__` method; I realise it might reduce complexity for > some ndarray subclasses eventually, but it also is an additional > maintenance burden. If you really think it is useful, I think it might > be more helpful to define a new mixin class which provides a version > of all indexing methods that just call `__numpy_getitem__` if that is > provided by the class that uses the mixin. I would *not* put it in > `ndarray` proper. I disagree that multiple inheritance (i.e. with your proposed mixin and ndarray) is something that numpy should enshrine in its API for subclasses. As the maintainer of an ndarray subclass, I'd much rather prefer just to implement a new duner method that older numpy versions will ignore. > > Indeed, the above might even be handier for subclasses, since they can > choose, if they wish, to implement a similar mixin for older numpy > versions, so that all the numpy version stuff can be moved to a single > location. (E.g., I can imagine doing the same for `__numpy_ufunc__`.) > > Overall, my sense would be to keep your PR to just implementing the > various new index methods (which are great -- I still don't really > like the names, but sadly don't have better suggestions...). > > But it might be good if others pipe in here too, in particular those > maintaining ndarray subclasses! > > All the best, > > Marten > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion > ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
Hi Nathan, The question originally posed is whether `ndarray` should provide that single method as a convenience already, even though it doesn't actually use it itself. Do you think that is useful, i.e., a big advantage over overwriting the new oindex, vindex, and another that I forget? My own feeling is that while it is good to provide some hooks for subclasses (__array_prepare__, wrap, finalize, numpy_ufunc), this one is too fine-grained and the benefits do not outweigh the cost, especially since it could easily be done with a mixin (unlike those other cases, which are not used to cover ndarray methods, but rather numpy functions, i.e., they provide subclasses with hooks into those functions, which no mixin could possibly do). All the best, Marten ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
p.s. Just to be clear: personally, I think we should have neither `__numpy_getitem__` nor a mixin; we should just get the quite wonderful new indexing methods! ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Mo, 2016-09-05 at 18:24 -0400, Marten van Kerkwijk wrote: > Hi Sebastian, > > It would seem to me that any subclass has to keep up to date with new > features in ndarray, and while I think ndarray has a responsibility > not to break backward compatibility, I do not think it has to protect > against new features possibly not working as expected in subclasses. > In particular, I think it is overly complicated (and an unnecessary > maintenance burden) to error out if a subclass has `__getitem__` > overwritten, but not `oindex`. > It is not complicated implementation wise to check for `__getitem__` existence. However, I start to agree that a warning icould be the better option. It might work after all. > For somewhat similar reasons, I'm not too keen on a new > `__numpy_getitem__` method; I realise it might reduce complexity for > some ndarray subclasses eventually, but it also is an additional > maintenance burden. If you really think it is useful, I think it > might > be more helpful to define a new mixin class which provides a version > of all indexing methods that just call `__numpy_getitem__` if that is > provided by the class that uses the mixin. I would *not* put it in > `ndarray` proper. > Yes, that is maybe a simplier option (in the sense of maintainability), the other would have a bit of extra information available. If this extra information is unnecessary, a MixIn is probably a bit simpler. > Indeed, the above might even be handier for subclasses, since they > can > choose, if they wish, to implement a similar mixin for older numpy > versions, so that all the numpy version stuff can be moved to a > single > location. (E.g., I can imagine doing the same for `__numpy_ufunc__`.) > You can always implement a mixin for older verions if you do all the logic yourself, but I would prefer not to duplicate that logic (Jaime wrote a python class that does it for normal arrays -- not sure if its 100% the same as I did, but you could probably use it in a subclass). So that a numpy provided mixin would not help with that supporting it in old numpy versions, I think. > Overall, my sense would be to keep your PR to just implementing the > various new index methods (which are great -- I still don't really > like the names, but sadly don't have better suggestions...). > Well... The thing is that we have to fix the subclasses within numpy as well (most importantly MaskedArrays). Of course you could delay things a bit, but in the end whatever we use internally could likely also be whatever subclasses might end up using. > But it might be good if others pipe in here too, in particular those > maintaining ndarray subclasses! > Yeah :). > All the best, > > Marten > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion > signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Mo, 2016-09-05 at 18:19 -0500, Nathan Goldbaum wrote: > > > On Monday, September 5, 2016, Marten van Kerkwijk ail.com> wrote: > > Hi Sebastian, > > > > It would seem to me that any subclass has to keep up to date with > > new > > features in ndarray, and while I think ndarray has a responsibility > > not to break backward compatibility, I do not think it has to > > protect > > against new features possibly not working as expected in > > subclasses. > > In particular, I think it is overly complicated (and an unnecessary > > maintenance burden) to error out if a subclass has `__getitem__` > > overwritten, but not `oindex`. > > > > For somewhat similar reasons, I'm not too keen on a new > > `__numpy_getitem__` method; I realise it might reduce complexity > > for > > some ndarray subclasses eventually, but it also is an additional > > maintenance burden. If you really think it is useful, I think it > > might > > be more helpful to define a new mixin class which provides a > > version > > of all indexing methods that just call `__numpy_getitem__` if that > > is > > provided by the class that uses the mixin. I would *not* put it in > > `ndarray` proper. > I disagree that multiple inheritance (i.e. with your proposed mixin > and ndarray) is something that numpy should enshrine in its API for > subclasses. As the maintainer of an ndarray subclass, I'd much rather > prefer just to implement a new duner method that older numpy versions > will ignore. > Hmm, OK, so that would be a + for the method solution even without the need of any of the extra capabilities that may be possible. > > > > Indeed, the above might even be handier for subclasses, since they > > can > > choose, if they wish, to implement a similar mixin for older numpy > > versions, so that all the numpy version stuff can be moved to a > > single > > location. (E.g., I can imagine doing the same for > > `__numpy_ufunc__`.) > > > > Overall, my sense would be to keep your PR to just implementing the > > various new index methods (which are great -- I still don't really > > like the names, but sadly don't have better suggestions...). > > > > But it might be good if others pipe in here too, in particular > > those > > maintaining ndarray subclasses! > > > > All the best, > > > > Marten > > ___ > > NumPy-Discussion mailing list > > NumPy-Discussion@scipy.org > > https://mail.scipy.org/mailman/listinfo/numpy-discussion > > > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Mo, 2016-09-05 at 21:02 -0400, Marten van Kerkwijk wrote: > p.s. Just to be clear: personally, I think we should have neither > `__numpy_getitem__` nor a mixin; we should just get the quite > wonderful new indexing methods! Hehe, yes but see MaskedArrays. They need logic to also index the mask, so `__getitem__`, etc. actually do a lot of things. Without any complex changes (implementing a unified method within that class or similar). The new indexing attributes simply cannot work right. And I think at least a warning might be in order (from the numpy side) for such a subclass. But maybe MaskedArrays are pretty much the most complex subclass available in that regard > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion > signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
Re: [Numpy-discussion] New Indexing Methods Revival #N (subclasses!)
On Mo, 2016-09-05 at 18:31 -0400, Marten van Kerkwijk wrote: > Actually, on those names: an alternative to your proposal would be to > introduce only one new method which can do all types of indexing, > depending on a keyword argument, i.e., something like > ``` > def getitem(self, item, mode='outer'): > ... > ``` Yeah we can do that easily. The only disadvantage is losing the square bracket notation. > > -- Marten > ___ > NumPy-Discussion mailing list > NumPy-Discussion@scipy.org > https://mail.scipy.org/mailman/listinfo/numpy-discussion > signature.asc Description: This is a digitally signed message part ___ NumPy-Discussion mailing list NumPy-Discussion@scipy.org https://mail.scipy.org/mailman/listinfo/numpy-discussion
