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

🗻 better error when flux monitor used in autograd #1917

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

tylerflex
Copy link
Collaborator

No description provided.

@tylerflex tylerflex added 2.7 will go into version 2.7.* .3 labels Aug 23, 2024
Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's OK for now even if it's the case, but does this handling mean that the error will only be raised after the forward simulation is done? If so it may be better to be able to catch errors like these in advance.

@tylerflex
Copy link
Collaborator Author

I thought about this, it would be great it we could, but we have no way of knowing if a FluxMonitor is going to be used in an objective function (and therefore triggering this error) without running the forward simulation.

If we want to do this, we'd need some way to more explicitly specify which monitors are going to be used in the objective function, but it makes the API clunkier.

Maybe this is something that can be solved with this approach of running an emulated simulation to figure out (a) what monitors are used and (b) how to set up the adjoint source. but let's leave this for another PR?

@momchil-flex
Copy link
Collaborator

Yep sounds good.

@tylerflex tylerflex merged commit 1c76897 into develop Aug 26, 2024
15 checks passed
@tylerflex tylerflex deleted the tyler/autograd_/fluxmnt_error branch August 26, 2024 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.7 will go into version 2.7.* .3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants