Added ShapeParam to store shapes #1045
Conversation
Seanny123
changed the title
|
LGTM |
|
To see this in action, see https://github.com/nengo/nengo_extras/blob/master/nengo_extras/convnet.py. It's been working well there. |
|
|
||
| def __set__(self, instance, value): | ||
| try: | ||
| value = tuple(int(v) for v in value) |
There was a problem hiding this comment.
My NumPy says about non-integer number in shapes "DeprecationWarning: using a non-integer number instead of an integer will result in an error in the future". I would say, in anticipation of this, we should not allow non-integers at all.
|
Add commits regarding my comment and to add a changelog entry. |
| try: | ||
| value = tuple(int(v) for v in value) | ||
| except TypeError: | ||
| if not all(is_integer(v) for v in value): |
There was a problem hiding this comment.
So the reason I didn't do this, is that I often do something like np.floor(a / b) to get some of my shape indices, and it's just convenient to not have to explicitly cast to an int. That said, maybe it's better to have to be explicit. But would it be worth checking if a floating point number is actually exactly an integer, and in that case just doing the cast ourselves?
There was a problem hiding this comment.
You wouldn't have that convenience in the future anyways: #1045 (comment)
I would be fine with doing the cast ourselves, but I am not sure how you can reliably detect whether a floating point number is an integer (considering rounding errors).
There was a problem hiding this comment.
You wouldn't have that convenience in the future anyways
I wouldn't necessarily have that convenience in Numpy (though it might be quite a while before they actually remove that), but I would still have it here.
So I think floor and ceil remove any rounding errors for reasonably sized numbers, but to really make a robust system we'd need to check tolerances or something, and that's all getting too complex. So I'm fine having to be explicit. That way, there's no magic going on, making it easier for users to debug.
There was a problem hiding this comment.
I wouldn't necessarily have that convenience in Numpy
But wouldn't the shape parameter usually be used to create a Numpy array?
There was a problem hiding this comment.
Perhaps, but not necessarily. Anyway, I'm fine with the explicit approach.
|
So one thing that got lost in @jgosmann's changes is that the input is no longer cast to a tuple. I had set this up so e.g. an iterable could be passed in and it would still work fine. So I think we need to override the |
I think we do this in |
|
Oh, right you are Ken. Ok, should be good to go! |
|
Cool, I'll go ahead now then! 🌈 |
Currently not used in Nengo, but will be used immediately in `nengo_extras`.
A variation of TupleParam that is specialized to storing shapes. Allows checking for the length of the shape, that all elements are ints (or castable to ints), and ensuring all values are greater than a given value (often 0 or 1).