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

Adding Shapes gives counterintuitive results #421

Closed
awygle opened this issue Jul 4, 2020 · 5 comments
Closed

Adding Shapes gives counterintuitive results #421

awygle opened this issue Jul 4, 2020 · 5 comments
Labels
Milestone

Comments

@awygle
Copy link
Contributor

awygle commented Jul 4, 2020

Shapes implement __add__, but adding them is currently not useful:

>>> from nmigen import *
>>> x = Signal(16)
>>> y = Signal(32)
>>> s = x.shape() + y.shape()
>>> s
(16, False, 32, False)

The expected result here (I argue) would be (48, False), but (probably because Shape is a named tuple under the hood) we get the not-very-useful result shown above.

Obviously in this case you can do (x+y).shape() but that's not always easy or possible (I hit a particular case involving an Enum)

@whitequark whitequark added the bug label Jul 4, 2020
@whitequark
Copy link
Member

So there are two unrelated issues here. First, that currently you can do shape + shape and the result is never desirable. We should fix that by not using namedtuple while still allowing to destructure shapes using width, signed = shape. (Is that possible? I think we can override __iter__ to do it...)

Second, the idea of overloading the + operation on shapes to do something not really related to addition. It doesn't seem like a good idea to me, but we can discuss it in a different issue.

@awygle
Copy link
Contributor Author

awygle commented Jul 4, 2020

I agree it's a bad idea. We should stop implementing extraneous operators for Shape, and that's all.

@awygle
Copy link
Contributor Author

awygle commented Jul 6, 2020

Should we preserve comparing Shapes to tuples of (int, bool)? This currently works (and is used in the test suite) but feels like a bit of a footgun.

@whitequark
Copy link
Member

This currently works (and is used in the test suite)

Where?

@whitequark
Copy link
Member

Oh sorry, you mean comparing for equality, not for ordering. Yeah let's preserve that, we can always deprecate it later if it turns out to cause issues.

@whitequark whitequark added this to the 0.3 milestone Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants