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

Parametric library #273

Merged
merged 56 commits into from
Jan 6, 2023
Merged

Parametric library #273

merged 56 commits into from
Jan 6, 2023

Conversation

znicolaou
Copy link
Collaborator

I think I'm ready to merge the new ParameterizedLibrary class and example notebook with master!

Changes to the core code are pretty minor. They include

  1. The new library class in feature_libraries/parameterized_library.py, which is really just a small wrapper around the GeneralizedLibrary for a specific use case. This case is when multiple trajectories are supplied that are governed by the same equations but with different parameter values. The parameter values are supplied as control input. It is important to construct the library features carefully in this case for PDE's, since we don't want to take spatial derivatives of the constant control parameters, so the library takes an input feature_library and an input parameter_library that act on the data and control input independently. The full library is essentially a TensoredLibrary of the two, but the GeneralizedLibrary enables different input features for the libraries.
  2. I added an exclude_libraries option to GeneralizedLibrary, which enables more control. In our case, we want to include only the tensor products of the input, and not concatenate the individual libraries as well, since that would lead to repeated columns in the usual case where the parameter_library has a constant bias term.
  3. I added a differentiation_method option to the PDELibrary and the WeakPDELibrary, to allow the user to change the differentiation method for the spatial derivatives. This addresses issue Add differentiation_method to PDELibrary and WeakPDELibrary #221
  4. I made a small change to the default behavior for AxesArray, so that if no axes are provided, the axes are comprehended with comprehend_axes.

Most of the work here is in the example notebook examples/17_pattern_formation/17_pattern_formation.ipynb, which has several examples including discrete maps, ODEs, and PDEs. Many of the examples are spatio-temporal and require pretty significant memory and cpu resources, so if you want to run them, you may want to do so on a cluster.

I'm hoping to merge this in the next week or so before we post an accompanying preprint on arxiv. Let me know if you want to see any details from the manuscript!

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2022

Codecov Report

Base: 92.39% // Head: 92.47% // Increases project coverage by +0.08% 🎉

Coverage data is based on head (46a8965) compared to base (a299cd8).
Patch coverage: 94.26% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   92.39%   92.47%   +0.08%     
==========================================
  Files          36       37       +1     
  Lines        3682     3736      +54     
==========================================
+ Hits         3402     3455      +53     
- Misses        280      281       +1     
Impacted Files Coverage Δ
pysindy/feature_library/sindy_pi_library.py 74.81% <ø> (ø)
pysindy/feature_library/weak_pde_library.py 94.88% <83.33%> (-0.24%) ⬇️
pysindy/feature_library/pde_library.py 95.21% <87.50%> (+1.77%) ⬆️
pysindy/pysindy.py 94.19% <91.66%> (-0.22%) ⬇️
pysindy/feature_library/generalized_library.py 95.76% <94.28%> (-1.61%) ⬇️
pysindy/feature_library/base.py 94.50% <95.45%> (+0.75%) ⬆️
pysindy/__init__.py 83.05% <100.00%> (+0.29%) ⬆️
pysindy/feature_library/__init__.py 100.00% <100.00%> (ø)
pysindy/feature_library/parameterized_library.py 100.00% <100.00%> (ø)
pysindy/differentiation/finite_difference.py 98.07% <0.00%> (-0.97%) ⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@akaptano
Copy link
Collaborator

Nice! I'll try to take a look this week. By the way, it would be nice to overhaul the GeneralizedLibrary notation anyways. The inputs_per_library and array to tell it which libraries to use for which variables is super confusing to use, and I get lots of questions on it. At the very least, it might be worth putting the indexing in the backend, and having the user interface be a lot easier to specify exactly what you want the libraries to do.

@znicolaou
Copy link
Collaborator Author

@akaptano totally agree the input_per_library is confusing and it would be easy to simplify it. I'll take a look while this is being reviewed and maybe sneak it in later.

Copy link
Member

@Jacob-Stevens-Haas Jacob-Stevens-Haas left a comment

Choose a reason for hiding this comment

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

Looks good! -just a consideration with the AxesArray choice, and a few list comprehensions.

docs/conf.py Outdated Show resolved Hide resolved
pysindy/feature_library/base.py Outdated Show resolved Hide resolved
test/feature_library/test_feature_library.py Outdated Show resolved Hide resolved
test/feature_library/test_feature_library.py Show resolved Hide resolved
requirements-dev.txt Show resolved Hide resolved
pysindy/feature_library/base.py Outdated Show resolved Hide resolved
pysindy/feature_library/generalized_library.py Outdated Show resolved Hide resolved
test/optimizers/test_optimizers.py Outdated Show resolved Hide resolved
test/optimizers/test_optimizers.py Outdated Show resolved Hide resolved
@znicolaou
Copy link
Collaborator Author

Ok! I think I resolved issues from @Jacob-Stevens-Haas -- thanks! Let me know if anything else looks off.

@znicolaou
Copy link
Collaborator Author

bump @akaptano -- let me know if you want to take a look at this one before I merge with master this week

@akaptano
Copy link
Collaborator

Going to take a look at this today for some documentation/other checks but not going to get into the technical details (need to spend time getting the other PR + paper ready still). If I have any suggestions I'll let you know by the end of the day!

…ut going to put rest of the comments in the PR request
@akaptano
Copy link
Collaborator

My main comment so far is that the example notebook could be substantially cleaned up in terms of:
(1) more text cells describing what is happening
(2) Some of the loading, data processing could be moved to a utils.py type file so that the notebook can be smaller + we want the example notebooks to be relatively easy to understand for users.
(3) It would be great if the notebooks could be reformatted according to pep8 + similar formatting to the current notebooks. For instance, the spacing for assignment and other operations everywhere should be ' = ' instead of '=' and so forth.

I started to do a little bit of this in a new commit but then realized this would be a big change so commenting here instead.

@akaptano
Copy link
Collaborator

Other random comments:

  1. Would be good to name the example notebook something more descriptive
  2. It would be good to add an example to the Example 1 notebook for a tiny demonstration of this new library. It might also be good to show a quick example in the Example 1 (or other notebook) using the "exclude_libraries" and "differentiation_method" changes you mentioned.

Anyways, this is awesome and my main comment is to clean up the notebook considerably. Let me know if you need a hand with anything, and sorry for my slowness on this!

@znicolaou
Copy link
Collaborator Author

Thanks for the feedback @akaptano! All very reasonable--I'll clean things up and organize some short examples for the other notebooks.

…. Added automatic reshaping and broadcasting to control parameter input and included short example in notebook 1 also.
…. Added automatic reshaping and broadcasting to control parameter input and included short example in notebook 1 also.
…. Added automatic reshaping and broadcasting to control parameter input and included short example in notebook 1 also.
@akaptano
Copy link
Collaborator

akaptano commented Jan 5, 2023

Hey Zack, the notebook is looking much better. I think you could probably move even more functions/plotting into the utils.py file, but this has a green light for me when you're happy to merge it in.

@znicolaou
Copy link
Collaborator Author

Thanks @akaptano ! I am pushing just one more clean up and am going to merge later tonight!

@znicolaou znicolaou merged commit f6b970b into master Jan 6, 2023
@znicolaou znicolaou deleted the parametric_library branch January 6, 2023 05:00
jpcurbelo pushed a commit to jpcurbelo/pysindy_fork that referenced this pull request Apr 30, 2024
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.

4 participants