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

pass kwargs along maybeview and dotview #31732

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

oxinabox
Copy link
Contributor

This is re #31729

@ararslan ararslan added the needs tests Unit tests are required for this change label Apr 15, 2019
@KristofferC KristofferC added the needs nanosoldier run This PR should have benchmarks run on it label Apr 15, 2019
@oxinabox
Copy link
Contributor Author

I guess to test this, the following would work?

struct DummyNamedDimArray<:AbstractArray end
Base.getindex(::DummyNamedDimArray; x) = x
@test Base.dotview(DummyNamedDimArray(); x=5) == 5

@mbauman
Copy link
Sponsor Member

mbauman commented Apr 16, 2019

Ok, I know I'm the one that put you up to this (and thanks for doing it!), but I'm not as certain it's a good idea anymore. The key thing for dotview is that it distinguishes between the scalar and nonscalar cases in an expression like A[X] .= Y.

If X is a single location like 5, then we broadcast Y into the object stored at A[5]. If, however, X is a collection of locations like 1:5, then we broadcast Y across A at those locations. In the case of a kwarg, we don't really know a priori which behavior to use.

For example, in your use-case, we'd want A[x=1:5] .= ... to use a view on the LHS and A[x=5] .= ... to use an unwrapped value.

Edit: I guess we could do some sort of pseudo-dispatch on the kwargs, but I don't think we really know enough about what the kwargs might mean to do that.

@oxinabox
Copy link
Contributor Author

@mbauman:
We should play around with this and get it sorted on NamedDims.jl
and then return to this PR then.
NamedDims.jl can be fully hyjacking dotview anyway, and probably will continue to since it needs to be 1.0 compatible.

If you want to try that out and make some issues, that would be very appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs nanosoldier run This PR should have benchmarks run on it needs tests Unit tests are required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants