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

Docs modifications to use QuadratureTraining instead of GridTraining #729

Merged
merged 143 commits into from
Oct 7, 2023

Conversation

sdesai1287
Copy link
Contributor

@sdesai1287 sdesai1287 commented Sep 3, 2023

solved #634

@@ -113,7 +113,7 @@ Then finally defining and optimizing using the `PhysicsInformedNN` interface.

```@example param_estim
discretization = NeuralPDE.PhysicsInformedNN([chain1, chain2, chain3],
NeuralPDE.GridTraining(dt), param_estim = true,
Copy link
Member

Choose a reason for hiding this comment

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

dt is still defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be used in other places in this file, thats why I didnt remove it

Copy link
Member

Choose a reason for hiding this comment

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

yes, dt is used for evenly spaced time grid for the data

Copy link
Member

Choose a reason for hiding this comment

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

but it's not required when changing to QuadratureTraining.

Copy link
Member

Choose a reason for hiding this comment

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

yes, it can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok I think I have fixed this one

@@ -65,7 +65,7 @@ function norm_loss_function(phi, θ, p)
end

discretization = PhysicsInformedNN(chain,
GridTraining(dx),
QuadratureTraining(),
Copy link
Member

Choose a reason for hiding this comment

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

dx is still defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as dt from the other file, it seems to be used in other places in this file, thats why I didnt remove it

Copy link
Member

Choose a reason for hiding this comment

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

But none of this file should be using it anymore?

Copy link
Contributor Author

@sdesai1287 sdesai1287 Sep 5, 2023

Choose a reason for hiding this comment

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

Ok I removed the instances of dx but it seems like the program is still creating a grid of some kind (see line 61 and below), maybe only for evaluation. I am assuming I don't want to delete all that stuff?

@sathvikbhagavan
Copy link
Member

Doc build is failing here and here

@sdesai1287
Copy link
Contributor Author

Doc build is failing here and here

Ok I fixed the issue for the second build fail, but I am not sure what the problem is for the first one

@sathvikbhagavan
Copy link
Member

Docs is still erroring at https://github.com/SciML/NeuralPDE.jl/actions/runs/6185760782/job/16791965357?pr=729#step:5:86
The doc build is using ComponentArrays@0.14.1
Any idea why it is failing? Does it need ComponentArrays@0.15?

@ChrisRackauckas
Copy link
Member

It at least wouldn't need a bump. What was the last version it was using wherei t was passing?

@sdesai1287
Copy link
Contributor Author

sdesai1287 commented Sep 25, 2023

This is passing for me locally without bringing up the ComponentArrays issue. @ChrisRackauckas Can we rebuild the CI or something? The the changes I made should not affect ComponentArrays

@ChrisRackauckas
Copy link
Member

I rebased onto master. Let's see how that goes.

@ChrisRackauckas
Copy link
Member

Just please set the ComponentArrays bound to what it needs to be so we can merge.

@sathvikbhagavan
Copy link
Member

I am not sure whats happening but I tried building the docs locally and it fails with the same error. But if I tried reducing the maxiters to 1200 from 2000 in https://github.com/SciML/NeuralPDE.jl/blob/master/docs/src/tutorials/constraints.md?plain=1#L91, it surprisingly passes.

@sathvikbhagavan
Copy link
Member

sathvikbhagavan commented Sep 28, 2023

The docs build is now failing because of IntegralsCubature 0.2.3 [also reproduced it locally] but it is passing with 0.2.2 locally. I haven't looked into the root cause.

The compat for ComponentArrays introduced in 87433bc isn't really affecting as it still uses 0.15.3 - https://github.com/SciML/NeuralPDE.jl/actions/runs/6323742800/job/17171915807?pr=729#step:4:96 but the other error we were seeing before is now passing.

@sathvikbhagavan
Copy link
Member

@sathvikbhagavan
Copy link
Member

@sdesai1287 can you rebase your branch on master? Compat bounds were introduced for IntegralsCubature in #743 where docs passed. It should pass here as well and then this can be merged.

@ChrisRackauckas
Copy link
Member

The docs successfully built on master, so I put this on top of a successful master.

Copy link
Member

@sathvikbhagavan sathvikbhagavan left a comment

Choose a reason for hiding this comment

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

@ChrisRackauckas docs build is passing. I think we can merge this.

@ChrisRackauckas ChrisRackauckas merged commit 4c79038 into SciML:master Oct 7, 2023
2 of 11 checks passed
@ChrisRackauckas
Copy link
Member

🎉

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.

5 participants