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

Unify reshape routing to make extension easier #39123

Open
darsnack opened this issue Jan 6, 2021 · 5 comments
Open

Unify reshape routing to make extension easier #39123

darsnack opened this issue Jan 6, 2021 · 5 comments
Assignees
Labels
arrays [a, r, r, a, y, s]

Comments

@darsnack
Copy link

darsnack commented Jan 6, 2021

We ran into an issue trying to extend Base.reshape in FluxML/Flux.jl#1448. Ideally, someone should only have to overload

Base.reshape(x::MySpecialArray, dims::Dims)

to get insert their custom behavior. Currently, calls involving a single : bypass this method signature directly to the internal _reshape. I'd like to redefine this line to be:

reshape(parent::AbstractArray, dims::Tuple{Vararg{Union{Int,Colon}}}) = reshape(parent, _reshape_uncolon(parent, dims))

so that it routes through the signature above after the uncolon logic is applied. I see no real problems with this, but I wanted to create an issue before a PR to get told why I'm wrong.

@mbauman
Copy link
Member

mbauman commented Jan 6, 2021

This is actually something I've wanted to do for a long time. It's a bit of a rats nest right now, and IIRC there are two levels of redirection — one for colons and one for axes/offset arrays. It's the latter that poses the bigger problem.

@darsnack
Copy link
Author

darsnack commented Jan 6, 2021

I assume the second level is this? So would you want to lift that logic out of an internal function?

@mbauman
Copy link
Member

mbauman commented Jan 6, 2021

Yes, I'd think so. But it's a general problem with Offset arrays — general array types want the "extension" method to be the simple ::Dims, whereas offset arrays require ::Axes. It makes things go in circles.

@darsnack
Copy link
Author

darsnack commented Jan 6, 2021

Okay I see why there was this extra path now. There's a dispatch ambiguity if you redefine it like I want mentioned above. Maybe we can just tweak _reshape_uncolon to no-op on the no colon case? Then we just remove reshape(parent, ::Dims).

@darsnack
Copy link
Author

darsnack commented Jan 9, 2021

It looks like there was already a fix for the initial complaint. I can still keep this issue open since we still want to clean up the interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

No branches or pull requests

3 participants