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

energies: adding energies to PDESystems #2256

Closed
wants to merge 2 commits into from

Conversation

drsk0
Copy link

@drsk0 drsk0 commented Sep 12, 2023

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.

We add a new field 'energies' to PDESystems to allow for additional loss
functions based on user defined energies of the system.
@@ -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,
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

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

Copy link
Author

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.
@ChrisRackauckas
Copy link
Member

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?

@drsk0
Copy link
Author

drsk0 commented Sep 15, 2023

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 eqs and bcs fields, because most commonly you will want to integrate over the whole domain, not just the boundary. The code to support it in NeuralPDE would indeed be easier, it's essentially only changing the equation parser.

I'd be happy with both solutions but clearly favor the first.

@ChrisRackauckas
Copy link
Member

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

@drsk0
Copy link
Author

drsk0 commented Sep 18, 2023

Another idea that came to mind is to extend the PhysicsInformedNN struct in NeuralPDE with a symbolic_additional_loss field. This would require no change to PDESystem and would set the energies apart from the bcs. But I'm also happy to allow energies in the bcs if you find the UX acceptable.

@drsk0
Copy link
Author

drsk0 commented Sep 29, 2023

@ChrisRackauckas @xtalax I'd like to give this a push. I see these options:

  1. Put energies in PDESystem. The field might be useless for other solvers like MethodOfLines.
  2. Allow energies in the boundary conditions/equations. Then there is no need to change PDESystem but other solvers like MethodOfLines might not be able to deal with such an equation.
  3. Have an additional_symbolic_loss in PhysicsInformedNN. No need to change PDESystem and no interference with other methods that are not suitable for energies.
  4. No energies at all.

I think 3) would be the best solution. Could you make a decision on which way you want to go?

@ChrisRackauckas
Copy link
Member

I would prefer 3 for this as well.

@drsk0
Copy link
Author

drsk0 commented Sep 30, 2023

I close this in favor of option 3) above and will adapt SciML/NeuralPDE.jl#734 accordingly.

@drsk0 drsk0 closed this Sep 30, 2023
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.

None yet

3 participants