[WIP] API for BNNODE #724

AstitvaAggarwal · 2023-08-18T06:03:09Z

for issue #719

ai-maintainer

AI-Maintainer Review for PR - [WIP] API for BNNODE

Title and Description ⚠️

Title and Description Need More Detail

The title of the pull request suggests that the changes are related to implementing an API for the BNNODE class. However, the description, which simply states "for issue #719," does not provide much context or explanation about the purpose of the changes. It would be helpful to have a more detailed description that explains the motivation behind the API implementation and how it addresses the specific issue.

Scope of Changes ❓

Scope of Changes May Be Too Broad

Based on the provided diff, it appears that the changes are focused on implementing the API for the BNNODE class. However, there is also a mention of the BNNPDE class and some related functions. It is not clear whether these additional changes are part of the same issue or if they are addressing multiple issues simultaneously. It would be helpful to have more information or context to determine if the changes are narrowly focused or if they encompass multiple issues.

Documentation ⚠️

Missing Docstrings

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

struct BNNODE <: NeuralPDEAlgorithm
struct BNNPDE <: NeuralPDEAlgorithm
function DiffEqBase.__solve(prob::DiffEqBase.AbstractODEProblem, alg::BNNODE,args...; )

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

Testing ⚠️

No Information on Testing

The description does not mention anything about how the author tested the changes. It would be beneficial to include information about the testing approach, such as the specific test cases used, the testing environment, and any relevant test results or observations. This would provide transparency and help reviewers understand the extent of testing performed on the changes.

Suggested Changes

Please provide a more detailed description of the changes and how they address issue API for BNNODE. #719.
If the changes related to the BNNPDE class and related functions are addressing a different issue, please consider separating them into a different pull request.
Add docstrings to the BNNODE and BNNPDE structs and the DiffEqBase.__solve function to describe their behavior, arguments, and return values.
Include information about how you tested the changes, including specific test cases used, the testing environment, and any relevant test results or observations.

Reviewed with AI Maintainer

ai-maintainer

AI-Maintainer Review for PR - [WIP] API for BNNODE

Title and Description ⚠️

Title and Description Need More Detail

The title of the pull request indicates that the changes are related to implementing an API for BNNODE. However, the description is not detailed and does not provide enough context about the changes. Please provide a more comprehensive description that explains the purpose of the changes, how they address the issue, and any other relevant information.

Scope of Changes 👍

Changes are Narrowly Focused

The changes appear to be narrowly focused on implementing the API for BNNODE. However, without additional context or information, it is difficult to determine if the changes are addressing multiple issues simultaneously or if they are solely focused on one specific issue. Please clarify this in the description.

Testing ⚠️

No Information on Testing

The description does not provide any information about how the changes were tested. Please include details about the testing approach, including any unit tests, integration tests, or specific scenarios that were tested to validate the changes.

Documentation ⚠️

Missing Docstrings

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

struct BNNODE <: NeuralPDEAlgorithm
struct BNNPDE <: NeuralPDEAlgorithm
function DiffEqBase.__solve(prob::DiffEqBase.AbstractODEProblem, alg::BNNODE,args...; )

Please add docstrings to these entities to describe their behavior, arguments, and return values.

Code Changes ⚠️

Missing Newline at End of File

Please add a newline at the end of the file as per standard coding conventions.

Potential Issues ⚠️

Check for Neural Network Type

The code checks if `BNNODE.chain` is a `Lux.AbstractExplicitLayer` or a `Flux.Chain`, and throws an error if it is neither. However, it would be better to check this at the start of the function and exit early if the condition is not met. This would avoid unnecessary computation and make the code more efficient.

Reviewed with AI Maintainer

xtalax · 2023-08-21T14:56:56Z

Looking good, I wanna see your tests passing and documentation with an example then lgtm

should work

…E.jl into develop

AstitvaAggarwal · 2023-08-29T21:40:17Z

@Vaibhavdixit02 @xtalax on my local machine all tests pass(v1.9.3) but for the v1 and v1.6 marginal errors crop up, what should I do? Using earlier versions affects performance. Also the v1.6 logging CI tests gives the statistics compat error as the AHMC version I'm using(NeuralPDE allows v0.5) works only with the latest Julia versions(refer https://julialang.slack.com/archives/CN04R7WKE/p1693333431525009?thread_ts=1693333431.525009&cid=CN04R7WKE).

Vaibhavdixit02 · 2023-08-29T21:50:23Z

Maybe we can drop 1.6 for this test group (it wouldn't be straightforward though). I don't recall why the compat was needed, is it possible to get rid of that?

Vaibhavdixit02 · 2023-08-30T01:34:03Z

Looking at test failures the values seem to be in the right ballpark so my guess is this is just a numerical difference between your local runs and the CI machine. You are probably not on the same OS (linux)? Relax the tolerances of the tests

AstitvaAggarwal · 2023-08-30T02:56:11Z

Maybe we can drop 1.6 for this test group (it wouldn't be straightforward though). I don't recall why the compat was needed, is it possible to get rid of that?

was done here by @ChrisRackauckas .

AstitvaAggarwal · 2023-08-30T03:01:31Z

whoa! after this update in Build to the latest Julia version the tests are almost exactly how i wanted them to be!

AstitvaAggarwal · 2023-09-01T08:19:39Z

@Vaibhavdixit02 ive added the "missing" Likelihood term i was talking about, if you want to run it with the regular tests just uncomment line 53 in the file "advancedHMC_MCMC.jl", ive tried, it works!

src/advancedHMC_MCMC.jl

…ce for special likelihood term

test/BPINN_Tests.jl

Need to update docs for diff training strategies,etc

xtalax · 2023-09-15T12:08:07Z

@Vaibhavdixit02 lgtm?

Vaibhavdixit02 · 2023-09-15T13:12:55Z

src/BPINN_ode.jl

+
+    function BNNODE(chain, Kernel = HMC; strategy = nothing,
+        draw_samples = 2000,
+        priorsNNw = (0.0, 2.0), param = [nothing], l2std = [0.05],


Why is this [nothing] and not just nothing?

Vaibhavdixit02 · 2023-09-15T13:13:50Z

src/BPINN_ode.jl

+        phystd = [0.05], dataset = [nothing],
+        init_params = nothing,
+        physdt = 1 / 20.0, nchains = 1,
+        autodiff = false, Integrator = Leapfrog,


All the NUTS kwargs should go into a struct or a namedtuple

Vaibhavdixit02 · 2023-09-15T13:16:35Z

A couple of minor things, but they can be addressed in a different PR since this one's quite big already. So LGTM

AstitvaAggarwal · 2023-09-15T13:24:37Z

Thank you 🎉

New PR

dcbca1a

ai-maintainer bot reviewed Aug 18, 2023

View reviewed changes

AstitvaAggarwal marked this pull request as draft August 18, 2023 06:03

Almost done ig

3aa50b4

AstitvaAggarwal marked this pull request as ready for review August 20, 2023 14:35

ai-maintainer bot reviewed Aug 20, 2023

View reviewed changes

ready player 1

a0bfa6b

AstitvaAggarwal and others added 22 commits August 23, 2023 00:54

added docs, minor changes, more tests

823048b

prev tests did not pass the vibe check

df3658a

Merge branch 'SciML:master' into develop

c219745

tests

4fbef0f

Update BPINN_Tests.jl

a78dcac

should work

test should pass

7b08e86

Merge branch 'develop' of https://github.com/AstitvaAggarwal/NeuralPD…

038beb3

…E.jl into develop

ready player one

31f0bbe

reduced iters

27609ce

more changes

a812c8b

optimizing tests

b700abd

yuh

04088bb

.......

4d944ab

pls work man

5123ea7

|TT|

30605cd

[TT]

9f12449

statistics dependancy compatib

e48915f

im back

7e399c9

Flux changes

d9643ae

.

8ae92f6

less std for weights

4ce8666

less std for weights

41bc26b

AstitvaAggarwal added 2 commits August 29, 2023 23:13

minor changes

f227012

Julia versions affect accuracy

a09de44

AstitvaAggarwal added 2 commits September 1, 2023 11:06

Added my suggested missing Loss function part, adjusted tests

09c0620

minor changes

51ecb37

Vaibhavdixit02 reviewed Sep 1, 2023

View reviewed changes

src/advancedHMC_MCMC.jl Outdated Show resolved Hide resolved

Vaibhavdixit02 reviewed Sep 1, 2023

View reviewed changes

src/advancedHMC_MCMC.jl Outdated Show resolved Hide resolved

added example, fixed multi dependant variable case, verfied performan…

b814a93

…ce for special likelihood term

Vaibhavdixit02 reviewed Sep 2, 2023

View reviewed changes

test/BPINN_Tests.jl Outdated Show resolved Hide resolved

AstitvaAggarwal and others added 9 commits September 2, 2023 22:48

fixed tests

83acfed

float 64 flux layers

ae3fa74

Merge branch 'SciML:master' into develop

dc4a839

now uses diff training strategies,

a9a4c1b

Need to update docs for diff training strategies,etc

tests

9cfc94d

relaxed tests

f7c2e2b

relaxed tests

e818ec8

added docs

0488e50

Merge branch 'SciML:master' into develop

d4b7218

Vaibhavdixit02 reviewed Sep 15, 2023

View reviewed changes

xtalax merged commit c0d6eef into SciML:master Sep 15, 2023
14 of 18 checks passed

AstitvaAggarwal deleted the develop branch September 15, 2023 13:33

AstitvaAggarwal mentioned this pull request Sep 15, 2023

fix NUTS kwargs handling and Default ode params vector #737

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] API for BNNODE #724

[WIP] API for BNNODE #724

AstitvaAggarwal commented Aug 18, 2023

ai-maintainer bot left a comment •

edited

Loading

ai-maintainer bot left a comment •

edited

Loading

xtalax commented Aug 21, 2023

AstitvaAggarwal commented Aug 29, 2023 •

edited

Loading

Vaibhavdixit02 commented Aug 29, 2023

Vaibhavdixit02 commented Aug 30, 2023

AstitvaAggarwal commented Aug 30, 2023 •

edited

Loading

AstitvaAggarwal commented Aug 30, 2023

AstitvaAggarwal commented Sep 1, 2023 •

edited

Loading

xtalax commented Sep 15, 2023

Vaibhavdixit02 Sep 15, 2023

Vaibhavdixit02 Sep 15, 2023

Vaibhavdixit02 commented Sep 15, 2023 •

edited

Loading

AstitvaAggarwal commented Sep 15, 2023

[WIP] API for BNNODE #724

[WIP] API for BNNODE #724

Conversation

AstitvaAggarwal commented Aug 18, 2023

ai-maintainer bot left a comment • edited Loading

Choose a reason for hiding this comment

AI-Maintainer Review for PR - [WIP] API for BNNODE

Title and Description ⚠️

Scope of Changes ❓

Documentation ⚠️

Testing ⚠️

Suggested Changes

ai-maintainer bot left a comment • edited Loading

Choose a reason for hiding this comment

AI-Maintainer Review for PR - [WIP] API for BNNODE

Title and Description ⚠️

Scope of Changes 👍

Testing ⚠️

Documentation ⚠️

Code Changes ⚠️

Potential Issues ⚠️

xtalax commented Aug 21, 2023

AstitvaAggarwal commented Aug 29, 2023 • edited Loading

Vaibhavdixit02 commented Aug 29, 2023

Vaibhavdixit02 commented Aug 30, 2023

AstitvaAggarwal commented Aug 30, 2023 • edited Loading

AstitvaAggarwal commented Aug 30, 2023

AstitvaAggarwal commented Sep 1, 2023 • edited Loading

xtalax commented Sep 15, 2023

Vaibhavdixit02 Sep 15, 2023

Choose a reason for hiding this comment

Vaibhavdixit02 Sep 15, 2023

Choose a reason for hiding this comment

Vaibhavdixit02 commented Sep 15, 2023 • edited Loading

AstitvaAggarwal commented Sep 15, 2023

ai-maintainer bot left a comment •

edited

Loading

ai-maintainer bot left a comment •

edited

Loading

AstitvaAggarwal commented Aug 29, 2023 •

edited

Loading

AstitvaAggarwal commented Aug 30, 2023 •

edited

Loading

AstitvaAggarwal commented Sep 1, 2023 •

edited

Loading

Vaibhavdixit02 commented Sep 15, 2023 •

edited

Loading