Incorporating given points into training

[sdesai1287]

#644 This is working for the given example

[ai-maintainer]

AI-Maintainer Review for PR - Initial commit of given_points_training

Title and Description ⚠️

Title and Description could be improved

The title of the pull request, "Initial commit of given_points_training," is somewhat descriptive but could be more explicit about the changes introduced. The description, "#644 This is working for the given example," is not very informative. It would be helpful to provide more context about the changes, such as what "given points training" entails and what problem it solves. Please consider revising the title and description to provide more clarity.

Scope 👍

The changes are narrowly focused

The changes in this pull request are narrowly focused on the implementation of the "given points training" feature. The changes are primarily related to this feature, and there are no indications that the author is trying to resolve multiple issues simultaneously.

Testing ⚠️

Testing details are missing

The description does not provide any information about how the changes were tested. It would be helpful to include details about the testing process, such as the test cases used, the expected results, and any specific testing frameworks or methodologies employed.

Documentation ⚠️

Some functions and methods are missing docstrings

The following functions, classes, or methods do not have docstrings:

• inner_loss_DAE
• generate_loss_DAE
• GivenPointsTraining
• get_loss_function (with strategy::GivenPointsTraining argument)

These entities should have docstrings added to describe their behavior, arguments, and return values.

Suggested Changes

• Please revise the title and description to provide more clarity about the changes and their purpose.
• Please provide details about how the changes were tested.
• Please add docstrings to the inner_loss_DAE, generate_loss_DAE, GivenPointsTraining, and get_loss_function (with strategy::GivenPointsTraining argument) functions, classes, or methods.

Reviewed with AI Maintainer

[ChrisRackauckas]

As mentioned in #644, tstops would be the right way to do this. Basically, tstops is a common arg https://docs.sciml.ai/DiffEqDocs/stable/basics/common_solver_opts/ that forces a solver to take into account those points. Those would be your "given points" in this sense for an NNODE. So what we'd want to do is take any strategy and then augment the points from that strategy with the tstops vector at every training stage.

This would be a lot more useful since only using the given points is likely never a good idea. The points should be changing and randomized so that just won't be possible with given points. But what we can do is do for example a QuadratureStrategy but, knowing the point of a discontinuity, add a few training points around this point that we know is going to be difficult.

[sdesai1287]

Ok got it, we do not want a separate training strategy, we want to add this functionality to each training strategy. I've written up some stuff so far, but there are some issues currently. It works well for WeightedIntervalTraining and GridTraining. For StochasticTraining it is telling me I cannot mutate an array (the points array to add the given points) because that is a limitation of Zygote has. I am currently reading up about Zygote and I should be able to solve this issue.

[ChrisRackauckas]

You don't need to mutate or add to the loss function code of any strategy. Just append the values by adding the loss.

[sdesai1287]

I think this is what you are referring to? I tested it with 3 training strategies and it turned a failing solver into a successful solver in all 3 cases

[ChrisRackauckas]

This looks close, just needs the clean ups mentioned.

[ChrisRackauckas]

Why make a separate test group? Does this one really take half an hour?

[sdesai1287]

Why make a separate test group? Does this one really take half an hour?

It does not take half an hour, I just fixed that now and just made it run as part of 'All'

[ChrisRackauckas]

All is not a test group. Please put it with the other nnode tests

[sdesai1287]

All is not a test group. Please put it with the other nnode tests

Fixed now, sorry for the confusion

[ChrisRackauckas]

Test fails

[sdesai1287]

Weird, it was working on my end when I ran it, but I made some edits and accounted for the relative size of the tstops entry when calculating the total loss. This works much more consistently now. I am not sure if this is exactly what we were looking for with this idea but hopefully it does what we want

[ChrisRackauckas]

No, read the CI... it was missing test imports.

[sdesai1287]

No, read the CI... it was missing test imports.

Oh maybe I misinterpreted what you said, I thought you meant it was failing the tests I wrote. But after reading the CI it says that its missing IntegralsCuba (which I never touched) and I haven't changed the manifest.toml or project.toml.

[ChrisRackauckas]

No, it's saying OptimizationPolyalgorithms isn't a test dependency.

[sdesai1287]

No, it's saying OptimizationPolyalgorithms isn't a test dependency.

Ok I removed it from the file, hopefully everything is good now

[ChrisRackauckas]

https://buildkite.com/julialang/neuralpde-dot-jl/builds/1754#018a164a-4217-4906-bb48-31b96d18ed21/6-13280

You never loaded it.

[sdesai1287]

https://buildkite.com/julialang/neuralpde-dot-jl/builds/1754#018a164a-4217-4906-bb48-31b96d18ed21/6-13280

You never loaded it.

oops, loaded now

[sdesai1287]

.

not really sure where these are coming from in the buildkite