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

Only directly block mutation on Arrays #812

Closed
wants to merge 1 commit into from
Closed

Conversation

oxinabox
Copy link
Member

@oxinabox oxinabox commented Oct 22, 2020

Any other arrray is going to be implemented in terms of an Array.
So if it is mutated then it will eventually mutate an an internal Array typed field and thow this error (if we have to AD that far).
But if we don't endup having to AD that far some how, then we should not error.

Also fixes the primal for popfirst! etc.
Though that doesn't matter since it will error anyway.

I was going to block it for BitArray also but then I realized that we should be treating BitArrays are nondifferentiable anyway
JuliaDiff/ChainRules.jl#293

@mcabbott
Copy link
Member

How about CuArrays, and MArrays, perhaps SparseArrays? The first are StridedArrays, at least.

But what's an example where you (try to) mutate some container, but not the array?

@oxinabox
Copy link
Member Author

SparseArrays have Arrays under the hood.
I think MArrays do utter evil under the hood that will error for it's own reasons I expect.

CuArrays don't support setindex! in the first place do they?
If they do they also will error for their own reasons.

@mcabbott
Copy link
Member

CuArrays give a warning on scalar setindex!, by default.

But what's an example where you mutate some AbstractArray, which does not end up mutating the underlying Array?

@oxinabox
Copy link
Member Author

But what's an example where you (try to) mutate some container, but not the array?

I don't think there is one.
I do want to tighten up how Zygote over-generally defines it's rules.
My instinct is this will help make errors appear in more relevant places.

Related to this, why don't we throw this error during the forward pass?
That would give a more easy to read stack-trace, no?

@mcabbott
Copy link
Member

I guess it's conceivable that someone calls fwd, _ = pullback(f, x) and then (for some of their fs) does not call back. But this seems pretty contrived, and a forward pass error would (I presume?) have a clearer stacktrace, which would be great.

If there's no case in which not erroring on setindex!(::AbstractArray, ... allows some desired behaviour, then all it that does is replaces an error with a warning on CuArrays (and perhaps with some other error, or else with a wrong answer?), and make the error from SparseMatrix come from deeper inside.

@CarloLucibello
Copy link
Member

closing as stale

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