-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
energies: adding energies to PDESystems #2256
Conversation
We add a new field 'energies' to PDESystems to allow for additional loss functions based on user defined energies of the system.
src/systems/pde/pdesystem.jl
Outdated
@@ -85,7 +87,7 @@ struct PDESystem <: ModelingToolkit.AbstractMultivariateSystem | |||
gui_metadata: metadata for MTK GUI. | |||
""" | |||
gui_metadata::Union{Nothing, GUIMetadata} | |||
@add_kwonly function PDESystem(eqs, bcs, domain, ivs, dvs, | |||
@add_kwonly function PDESystem(eqs, energies, bcs, domain, ivs, dvs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please include a default value, and make it an optional argument so as to not break all other pdesystems written to date
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @xtalax, I tested that this won't break a pdesystem without energies. But I think a default value would be a good idea for all of the fields eqs
, energies
and bcs
. Would you agree to add a default value (empty vector) to these three fields?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A PDE without eqs is undefined so I'm ok with this erroring. Default for bcs and energies is sensible though. Please remember to add test cases for this and also the energy free case in test/pde.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho a PDE without eqs
is actually well-defined as well. It just means that there are no relations in the jet bundle. For example, you could just solve for a given boundary condition and low energy or even just for low energy solutions or solutions only satisfying the boundary conditions. If all three of eqs
, bcs
and energies
are empty, the semantics should be that any function defined on the domain is returned - the neural network with randomly initialized weights.
I'll add defaults for bcs
and energies
and tests for now. Please note that this PR only makes sense if SciML/NeuralPDE.jl#734 is accepted as well.
This makes energies an optional keyword so not to break PDESystems generated without them. Tests are added to test/pde.jl.
I'm not convinced this needs to be separated from BCs. I do not see a tangible benefit to any solver with it separate. Can someone demonstrate a case where it actually helps? |
My reasoning for having a separate field for energies is that the presentation of the PDE problem in code will be cleaner and easier to understand this way. An energy associated with a PDE is quite a different concept from a boundary condition in my view. I also think that errors will be easier to analyze and track. If we wouldn't add a new field, I'd actually expect to be able to put an energy integrand in both, the I'd be happy with both solutions but clearly favor the first. |
But how do we make use of it in MethodOfLines and other extensions? What's the more general plan here? And it's still not clear that it actually makes NeuralPDE better than just treating them as a BC |
Another idea that came to mind is to extend the |
@ChrisRackauckas @xtalax I'd like to give this a push. I see these options:
I think 3) would be the best solution. Could you make a decision on which way you want to go? |
I would prefer 3 for this as well. |
I close this in favor of option 3) above and will adapt SciML/NeuralPDE.jl#734 accordingly. |
We add a new field 'energies' to PDESystems to allow for additional loss functions based on user-defined energies of the system.
This PR is a prerequisite of SciML/NeuralPDE.jl#734.