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

Inverse Dirichlet Adaptive Loss #504

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

hpieper14
Copy link

Adds inverse dirichlet adaptive loss function to address #500

"""

mutable struct InverseDirichletAdaptiveLoss{T <: Real} <: AbstractAdaptiveLoss
reweight_every::Int64
Copy link
Member

Choose a reason for hiding this comment

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

Rename this reweights_steps? That's a bit more consistent with the rest of the ecosystem

Copy link
Author

Choose a reason for hiding this comment

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

I chose reweight_every to be consistent with the variable names chosen for MiniMaxAdaptiveLoss and GradientScaleAdaptiveLoss. Is the naming convention is reweight_steps across other files?

Copy link
Member

Choose a reason for hiding this comment

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

progress_steps, timeseries_steps, p_steps, etc. It's not the biggest deal, but it's slightly more consistent with what else is out there in SciML.

pde_grads_std_max = maximum(pde_grads_std_all)
bc_grads_std = [std(Zygote.gradient(bc_loss_function, 0)[1]) for bc_loss_function in bc_loss_funcitons]

nonzero_divisor_eps = adaloss_T isa Float64 ? Float64(1e-11) : convert(adaloss_T, 1e-7)
Copy link
Member

Choose a reason for hiding this comment

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

What's the justification on the 1e-11 and 1e-7?

Copy link
Member

Choose a reason for hiding this comment

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

Bump on this.

Copy link
Author

Choose a reason for hiding this comment

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

I looked over the implementation by the authors of the paper and they don't include an additional epsilon in the denominator, so I think I should remove this.

@ChrisRackauckas
Copy link
Member

The biggest thing is that this needs tests.

@hpieper14
Copy link
Author

This seems to work ok for the 2D Poisson equation but it does not outperform the nonadaptive loss across several seeds.

@ChrisRackauckas
Copy link
Member

Try the incompressible Navier-Stokes from the paper. I think this may only make a difference when it's "sufficiently hard", and Poisson is simple enough that non-adaptive is fine.

@ChrisRackauckas
Copy link
Member

Some of the review comments haven't been addressed, but other than that I think this is pretty close to merging. Using the Poisson 2D as the test is fine for CI, but we should test it separately on something harder. That separating testing can then turn into a tutorial.

@hpieper14
Copy link
Author

I've started working on a tutorial/test for the incompressible Navier-Stokes and I'll add the tutorial to docs/src/pinn. Should I add the test to adaptive_loss_tests.jl too?

@ChrisRackauckas
Copy link
Member

Should I add the test to adaptive_loss_tests.jl too?

If it's not too long. That might get constrained by compute time when used in the test infrastructure.

Co-authored-by: Christopher Rackauckas <[email protected]>
Copy link
Member

@ChrisRackauckas ChrisRackauckas left a comment

Choose a reason for hiding this comment

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

This looks right to be. @zoemcc ?

@zoemcc
Copy link
Contributor

zoemcc commented Apr 29, 2022

This looks really good but I don't have time until my project deadline on Tuesday to review further.

@ChrisRackauckas
Copy link
Member

This should get rebased due to changes in #553. That would hopefully make it much cleaner too.

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.

3 participants