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

Support new_axes= keyword in atop #1612

Merged
merged 1 commit into from
Oct 5, 2016
Merged

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented Oct 4, 2016

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})

@mrocklin
Copy link
Member Author

mrocklin commented Oct 4, 2016

This could use high-level review on API. Can anyone think of a cleaner way to do this or attractive alternatives?

@jcrist @shoyer

@shoyer
Copy link
Member

shoyer commented Oct 4, 2016

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).

@mrocklin
Copy link
Member Author

mrocklin commented Oct 4, 2016

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?

Copy link
Member

@shoyer shoyer left a 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
Copy link
Member

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"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@mrocklin
Copy link
Member Author

mrocklin commented Oct 5, 2016

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.

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})
@shoyer
Copy link
Member

shoyer commented Oct 5, 2016

I don't think there's any conflict here. Every valid argument to your new_axes was also a valid argument for my out_chunks, but new_axes is certainly a more intuitive name. I think this can go in as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants