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

Adjoint Sensitivities don't work properly for domains that aren't ConstantTPDomain #84

Closed
mjohnson541 opened this issue Aug 26, 2020 · 4 comments
Labels
bug Something isn't working enhancement New feature or request Priority: High High Priority

Comments

@mjohnson541
Copy link
Collaborator

mjohnson541 commented Aug 26, 2020

After some discrepancies between sensitivities from forward sensitivity analysis and adjoint I suspect ReverseDiffVJP(true) isn't suitable for systems where the rate coefficients change, the catch is that ReverseDiffVJP(false) always gives me NaNs and TrackerVJP has some typing issues. There also seems to be a separate typing issue for ConstantTVDomain. The typing issues shouldn't be too difficult to resolve, it's worth noting that ReverseDiffVJP(false) is expected to be fastest for these cases so it's worth trying to get it to work properly. This has made it very clear we need a solid set of tests for adjoint sensitivities.

@mjohnson541 mjohnson541 added bug Something isn't working enhancement New feature or request labels Aug 26, 2020
@mjohnson541 mjohnson541 added the Priority: High High Priority label Aug 26, 2020
@mjohnson541 mjohnson541 added this to To do in Reaction Mechanism Simulator 1.0.0 via automation Aug 31, 2020
@ChrisRackauckas
Copy link
Contributor

Do you have an MWE on this? I'd like to take a look. Tape compilation requires that there aren't branches. When you say "rate coefficients change", that makes me thing you have a callback changing parameters, in which case you might need to use ReverseDiffAdjoint().

@mjohnson541
Copy link
Collaborator Author

Yes, so I've been digging through this the last few days. When I originally implemented I mostly focused on InterpolatingAdjoint, I was aware of the issues with ReverseDiffVJP(true), but that was the only thing that worked properly straight out of the box and while there is branching related to temperature and pressure I thought it would work in more cases that it seems to. ReverseDiffVJP(false) gives me NaNs, which I haven't investigated yet. TrackerVJP() gave me errors, which I've been working on recently. With TrackerVJP after fixing some array concatenation issues I've run into StackOverflowErrors, some of the rate coefficients aren't calculated vectorized, and I suspect through the operations they're causing the vectorized ones to convert to Array{TrackedReal}.

I've attached a jupyter notebook MWE, feel free to take a look. Honestly though I feel like at this stage I'm just trying to figure out how the Reverse mode tools are interacting with the code and trying to make sure it can handle the different types properly.

ConstantVH2Combustion_adjoint_MWE.ipynb.zip

@ChrisRackauckas
Copy link
Contributor

Interesting, I'll have to catch up on this in a bit. I wonder if those NaNs are true adjoint NaNs (as in bugs), or from numerical accumulation. It'll take some digging.

@mjohnson541
Copy link
Collaborator Author

Closing with merging of #95

Reaction Mechanism Simulator 1.0.0 automation moved this from To do to Done Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request Priority: High High Priority
Development

No branches or pull requests

2 participants