-
Notifications
You must be signed in to change notification settings - Fork 119
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
SINDy #167
SINDy #167
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome contribution Cody!
Few suggestions before we integrate this PR.
- lets change this PR to be integrated into develop branch first, this is our current convention for new features, we aggregate new PRs in develop before merging with master and releasing new version
- lets make the notebook a bit more didactical by adding more method explanations in markdown cells using more latex notation and figures to illustrate the methodology, please get inspired by our existing notebooks on NODEs, Koopman model, or others in the lib. We should also add citations to relevant papers for further reading
- Lets make sure we define all variables and parameters explicitly in the text.
- Lets break down and document the Library construction cell into more detail. Lets motivate the use of fourier library functions and the choice of custom functions. Would be nice to plot the functions in the library to give the user visual intuition.
- In the markdown, I suggest introducing the new class FunctionLibrary what is its purpose, main attributes, methods, and data types required.
- Lets also expand docstrings and specify datatypes for all the class init parameters. Specifically,
:param functions: (list)
- is this a list of np expressions or can we use torch expressions. - Right now the implementation of the FunctionLibrary is using np.arrays, I suggest using only torch to avoid issues.
- In the SINDY model cell I suggest providing reference to the UDE notebook and paper and making the link by saying that this proposed implementation of SINDY is a special case of the UDE model. We can also mention that we can also use adjoint method to solve the SINDY model forward in time by referencing NODE model. This will make connections to other established metods.
- it would be great to discuss the differences between this implementation using SGD and the implementation in the paper using SQTLS
- in the results, lets compare identified expression and coefficients with ground truth
Jan, the function library uses np.arrays primarily because torch.Tensors cannot be used to store arbitrary functions. We could switch to using simple python lists exclusively to avoid confusion between numpy and torch. |
in that case lets use python lists unless we are losing expressivity. |
Change type of function container to list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!
Final comments:
- Lets rename the SINDyExample.ipynb to Part_9_SINDy.ipynb
- lets update the readme to add hyperlink to google colab
- in the Model Integration section of your example you mentioned adjoint solver for integrating the ODE - this would correspond to the torchdiffeq integrator, however, in the code you use RK4 integrator that is using automatic differentiation through unrolled operations of the integrator - this is just a technical detail but it would be good to clarify.
Change additional markdown descriptions to avoid confusion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great contribution!
Incorporate SINDy into Neuromancer. Create example for solving for RHS of Thomas Attractor using a SINDy model.