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

Added a separate package BayesianNeuralPDE.jl #920

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

Conversation

ParamThakkar123
Copy link

Checklist

  • Appropriate tests were added
  • Any code changes were done in a way that does not break public API
  • All documentation related to code changes were updated
  • The new code follows the
    contributor guidelines, in particular the SciML Style Guide and
    COLPRAC.
  • Any new documentation only uses public API

Additional context

Add any other context about the problem here.

@ParamThakkar123
Copy link
Author

The CI failed showing some python distutils error

@ParamThakkar123
Copy link
Author

@ChrisRackauckas The BayesianNeuralPDE part has been split into a separate package. Do I also need to make respective changes in the docs ? because the current documentation tests would likely fail on the CI while the others pass

@ParamThakkar123
Copy link
Author

The tests that failed are based on documentation. Should I make changes to that too??

@@ -22,6 +22,7 @@ IntervalSets = "8197267c-284f-5f27-9208-e0e47529a953"
LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e"
LogDensityProblems = "6fdf6af0-433a-55f7-b3ed-c6c6e0b8df7c"
Lux = "b2108857-7c20-44ae-9111-449ecde12c47"
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed"
Copy link
Member

Choose a reason for hiding this comment

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

Why was this added?

Copy link
Author

Choose a reason for hiding this comment

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

Because, when I precompiled NeuralPDE, it gave me an error saying this dependency is required for NeuralPDE but not found in its dependencies section

Copy link
Member

Choose a reason for hiding this comment

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

Interesting, it doesn't happen with me. Are you sure you are using NeuralPDE's env?

Copy link
Author

Choose a reason for hiding this comment

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

yes. I have made all the changes in the same repository and Julia env

@info "Running tests with $(RETESTITEMS_NWORKERS) workers and \
$(RETESTITEMS_NWORKER_THREADS) threads for group $(GROUP)"

ReTestItems.runtests(NeuralPDE; tags = (GROUP == "all" ? nothing : [Symbol(GROUP)]),
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be BayesainNeuralPDE? Also, this might not be tested at all in CI. Look at how its done in OrdinaryDiffEq or some other where there are subpackages.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. I will see to it

Copy link
Member

Choose a reason for hiding this comment

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

If you look at CI for ODEBPINN and PDEBPINN, no tests are run as it is removed from NeuralPDE. Idea is to run them whenever someone makes changes to the subpackage BayesianNeuralPDE. So you might need to make changes to CI.yml file in .github

Copy link
Author

Choose a reason for hiding this comment

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

Yes. Noted

@sathvikbhagavan
Copy link
Member

@ParamThakkar123, since #921 was merged, can you rebase this branch and finish this PR?

@ParamThakkar123
Copy link
Author

Sure

@ParamThakkar123
Copy link
Author

Also @sathvikbhagavan would like to have any suggestions you might have for SciML/SciMLBenchmarks.jl#1159

That PR is also almost done except the few errors that occur. I have identified some cases of its occurrence but would like to hear any suggestions you have further to fix that

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