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

Remove SArray rule that already exists in StaticArraysChainRulesCoreExt #124

Closed
wants to merge 7 commits into from

Conversation

simsurace
Copy link
Member

There is a warning that this rule is now superseded by https://github.com/JuliaArrays/StaticArrays.jl/blob/e2d772f9767abdcab20ce7ae6927dc25dc38714b/ext/StaticArraysChainRulesCoreExt.jl#L26-L30

As it is type piracy anyway, we should be able to gradually remove these rules.

@willtebbutt
Copy link
Member

Thanks for opening this. Any idea what's going on on 1.10?

@simsurace
Copy link
Member Author

No idea. Let's test on 1.9 as well.

@simsurace
Copy link
Member Author

So the "test gp" was due to a change in @test_broken, which now requires a Boolean. The other ones I'm not yet sure about.

@sefffal
Copy link

sefffal commented Jan 9, 2024

#121 is a dup then and can be closed.

@simsurace
Copy link
Member Author

#121 seems to go much further, I wouldn't close it just yet.

@simsurace
Copy link
Member Author

simsurace commented Jan 10, 2024

@willtebbutt I've spent some time on this but I don't think I can fix the issues. I tried to set all dependencies to the same version as they are resolved in the Julia 1.6 test, but to no avail. I don't understand why the error is triggered in one Julia version vs. the other, but I suspect it is due to the hacked-together nature of all the gradient tests.

@simsurace
Copy link
Member Author

Maybe we should ditch all the hacks, make a breaking release and start adding back AD functionality from there. Most likely, Enzyme is going to be easier to get working.

@willtebbutt
Copy link
Member

Maybe we should ditch all the hacks, make a breaking release and start adding back AD functionality from there. Most likely, Enzyme is going to be easier to get working.

I think I would be in favour of this -- it's become a complete nightmare to maintain. All of the functionality I've added for AD we well-motivated at the time, but it hard to justify now.

I wonder whether the right thing to do though is to just re-implement the functionality in certain key places, where we're confident that Enzyme is essential, and we know a lot about the types? For example, we could just produce an rrule for scan_emit using Enzyme for cases where we know the types well? (e.g. for certain concrete types of LGSSM, such as when all of the elements are concrete SArrays?)

My concern with going all-in on Enzyme at the minute is that I don't believe that it works in all of the places that we need it to. e.g. I don't think it has a rule for potrf! yet, which is a problem for anything involving Arrays.

@simsurace
Copy link
Member Author

Yeah, I did not battle test it with Enzyme yet, i.e. there are surely some things that do not work, so I don't think we can go all in at this moment. But with Zygote things are constantly breaking as well.
Maybe we can try to find compat upper bounds from back when all tests used to pass (basically the last release). As it is almost impossible to add new stuff now with things constantly breaking, it maybe wouldn't too bad if 0.x stays there and we can then start with a fresh minimal version (just inference, no expectation of training working for 1.0.0), and add AD support as we go.
One thing that is not clear to me is that if there is no concern for Zygote being able to analyze the code, maybe some things can be simplified and improved about the core algorithms.

@simsurace simsurace mentioned this pull request Jan 10, 2024
@simsurace
Copy link
Member Author

Closed in favor of #125

@simsurace simsurace closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants