dynamicslab / pysindy

A package for the sparse identification of nonlinear dynamical systems from data
https://pysindy.readthedocs.io/en/latest/
Other
1.36k stars 304 forks source link

Make AxesArray handle slicing correctly #451

Closed Jacob-Stevens-Haas closed 5 months ago

Jacob-Stevens-Haas commented 6 months ago

Fixes #220

It's long made sense to expose the AxesArray interface and document it. However, when it was created, it wasn't stable enough even for every use case inside pysindy. Indexing can remove, add, and even move around the axes in an array, which meant that properties such as ax_time and n_spatial were unreliable when an array was indexed. FiniteDifference._accumulate() is an example of great code that worked which AxesArrays, but in which AxesArray gave no extra understanding in debugging because the axis labels were incorrect.

This PR makes basic indexing, integer advanced indexing, and boolean array indexing work. It does so by adding a a private attribute that contains both forwards and backwards maps of the axes, and modifying __getitem__ to detect which axes are added, removed, combined, relocated, etc. The PR also expands the ufunc tests dramatically. Item 1 below is probably all that's needed to accept the PR.

Jacob-Stevens-Haas commented 6 months ago

CI bug related to https://bugs.python.org/issue42233, testing on 3.8/3.9 but it'll be an easy fix either way.

Jacob-Stevens-Haas commented 5 months ago

Heads up, I made AxesArray raise errors for broken axis labels (e.g. labeling a third axis in a 2D array). So numpy functions that change the axes of its inputs raise errors on AxesArray inputs. Previously, they just assigned the same axes as their inputs. I am fixing them one by one. So far I've finished reshape and transpose, did einsum but need to add tests , and need to do linalg.solve and tensordot.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (8341efb) 93.92% compared to head (bc3fbd2) 94.40%. Report is 2 commits behind head on master.

Files Patch % Lines
pysindy/utils/axes.py 98.28% 7 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #451 +/- ## ========================================== + Coverage 93.92% 94.40% +0.48% ========================================== Files 37 37 Lines 3602 3989 +387 ========================================== + Hits 3383 3766 +383 - Misses 219 223 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Jacob-Stevens-Haas commented 5 months ago

Issues to add to pysindy repo, discovered during this PR but not for this PR:

Jacob-Stevens-Haas commented 5 months ago

Alright @znicolaou, @akaptano, this is ready for your review, if desired! After this I'd suggest we release 2.0.0. One of the main points of this PR was to removes some defaults for AxesArray, so that I'm comfortable fixing issues without breaking backwards compatability.

This PR dramatically changes the AxesArray class to support slicing. This is the most common pysindy operation on arrays. Now that slicing works, AxesArray is stricter about creating arrays/array views. The strictness caused some functions in pysindy to fail; the PR patches FiniteDifference and PDELibrary as well as introduces implementations of common numpy operations on AxesArray (e.g. numpy.linalg.solve).

I'm going to leave this open for at least two weeks. I'd especially appreciate if you could take a look at the changes to the package outside of axes.py, which are many fewer and easier to review. Admittedly, some of these changes only make sense in debugging, when you can see that slices of the spatiotemporal grid carry the correct axis definition.