-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Support new_axes= keyword in atop #1612
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
Conversation
This looks like a slightly less general version of #1511? The ability to insert new axes is certainly more important than resizing chunks (I can no longer remember exactly what use case I had in mind there). |
8cf1487
to
e979541
Compare
Hrm, indeed. I had forgotten about #1511 . The solution presented here does less, but the changes are also more modest, which reduces the concerns about maintenance bloat a bit (though not to zero). I think the relevant question now is "Are we likely to want multi-chunk new dimensions?" If the answer is "yes" then we should think harder about this API. I'm actually unsure what adding a new multi-chunk dimension would look like. The user defined function doesn't have a clear way to output multiple chunks. Thoughts? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution presented here does less, but the changes are also more modest, which reduces the concerns about maintenance bloat a bit (though not to zero).
You also figure out a more elegant/minimal way to adjust top
than I did :).
I think the relevant question now is "Are we likely to want multi-chunk new dimensions?" If the answer is "yes" then we should think harder about this API.
My version didn't actually support mulit-chunking new dimensions. It supported changing the chunking of existing dimensions on the output while keeping "block indices" intact. This is a pretty natural sort of thing to do (adjusting the size of each chunk), but maybe more complex than warranted without clear use cases.
@@ -1750,6 +1753,8 @@ def atop(func, out_ind, *args, **kwargs): | |||
Block pattern of the output, something like 'ijk' or (1, 2, 3) | |||
concatenate: bool | |||
If true concatenate arrays along dummy indices, else provide lists | |||
new_axes: dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This (and concatenate) should go after *args
, and be marked as "keyword only"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Ah, I see. Is there a way to combine that use case into this one or a way to make this change future proof so that it doesn't conflict with future desired changes? |
Add new single-chunk dimensions with the ``new_axes=`` keyword, including the length of the new dimension. New dimensions will always be in a single chunk. >>> def f(x): ... return x[:, None] * np.ones((1, 5)) >>> z = atop(f, 'az', x, 'a', new_axes={'z': 5})
e979541
to
6dc7cbe
Compare
I don't think there's any conflict here. Every valid argument to your |
Add new single-chunk dimensions with the
new_axes=
keyword, includingthe length of the new dimension. New dimensions will always be in a
single chunk.