Open znicolaou opened 2 years ago
Sounds good zach! Two suggestions:
1) Maybe the calls to set_..._grid
should be a more functional vs object oriented design (e.g. create_..._grid()
). This allows us to test grid creation/validation much faster, without a pytest fixture of a feature library & the expensive fit_transform()
method.
2) If y
isn't needed, then we could call the code in transform()
. Not as sure about this, but lmk your thoughts: Our feature libraries are only feel like estimators to match the Pipeline interface, rather than actual estimators. The splitting of fit
and transform
seems an unnatural split, saving some intermediate arguments as object attributes, but this seems to break encapsulation (and costs extra memory, though it's probably minimal).
Is your feature request related to a problem? Please describe.
The
PDELibrary
andWeakPDELibrary
classes are initialized with a singlespatial_grid
andspatiotemporal_grid
that are used in derivative calculations, limiting the applicability ofmultiple_trajectories=True
to trajectories that are defined on the same grids.The current
spatial_grid
andspatiotemporal_grid
are also somewhat memory-wasteful now. We require the grids to be tensor product grids, so they actually only need N_dim vectors to specify rather than a tensor with N_dim+1 axes.Describe the solution you'd like
We should remove the grid arguments, and instead provide
set_spatial_grid
andset_spatiotemporal_grid
class functions, that take lists of grids. TheSINDy.fit
function should take optionalspatial_grid
andspatiotemporal_grid
options, and should invoke the libraryset_spatial_grid
andset_spatiotemporal_grid
at the time of the fit. The libraryfit
functions should then verify that the the providedx_full
has a compatible shape with the grid, and raise an error otherwise. A default grid can be used ifNone
is provide.Conserving the memory may not be necessary, since the data
x_full
will always be a similar size to the grids, so we can only reduce the data used by ~1/2.Additional context
This is a relatively simply fix, but will require changes to the notebooks and tests. I am deferring this for now so that we can complete #185