andgoldschmidt / derivative

Optimal numerical differentiation of noisy time series data in python.
https://derivative.readthedocs.io/en/latest/
Other
56 stars 8 forks source link

Derivative.d now handles negative axis arguments #43

Closed yb6599 closed 4 months ago

yb6599 commented 5 months ago

Hello, I am differentitating multidimensional arrays, I found that Derivative.d does not handle negative axis arguments. This PR introduces a small change that makes it handle negative arguments.

For example, using PySINDy SINDYDerivative,

t = np.arange(3)
x = np.ones((2, 3, 2))
differentiator = ps.SINDyDerivative(kind='finite_difference', k=1, axis=-2)
d = differentiator._differentiate(x, t)

I run into this error that originates at these lines in derivative.differentiation.py:

[251] extra_dims = tuple(length for ax, length in enumerate(orig_shape) if ax != axis)
[252] moved_shape = (orig_shape[axis],) + extra_dims
[253] dX = np.moveaxis(dX.T.reshape((moved_shape)), 0, axis)
[254] return dX

ValueError: cannot reshape array of size 12 into shape (3,2,3,2)

However, after the change introduced in this PR, it won't be an issue.

andgoldschmidt commented 5 months ago

Good catch, we definitely want this ability. Would you be willing to add a small test with this PR in case of future changes? It can be added to tests/test_interface.py.

andgoldschmidt commented 5 months ago

Also, it looks like the checks failed when trying to build docs and run tests, but I bet that's on our end.

yb6599 commented 5 months ago

@andgoldschmidt thanks for your reply, I've added the test.

andgoldschmidt commented 5 months ago

The changes look good, thanks! I am not sure why the github actions are failing here, but as soon as I debug that, I'll merge.

andgoldschmidt commented 4 months ago

@Jacob-Stevens-Haas these actions fail with ModuleNotFoundError: No module named 'importlib_metadata'. In this run, I added importlib_metadata directly instead of importlib.metadata, but this resulted in the screenshot errors (which look like missing features due to a version or function mismatch). Is there any obvious reason why the original action is failing?

Screenshot 2024-06-12 at 4 24 12 PM

On top of these issues, I was inspecting locally, and it looked like pytest didn't populate the entry points, so that this test failed:

def test_hyperparam_entrypoint(): ...
Jacob-Stevens-Haas commented 4 months ago

TL;DR: adding importlib-metadata to pyproject.toml should have fixed this, and it appears to work locally for me. Still looking into what might be the unexpected kwarg error, there's some clues though.

Broadly, importlib.metadata contains new metadata features that are provided to older python releases via the pip package importlib_metadata. I wanted to use a version of importlib.metadata that came out in 3.10, which is why I have the code block:

if sys.version_info < (3, 10):
    from importlib_metadata import entry_points
else:
    from importlib.metadata import entry_points

But I never added importlib-metadata (the distribution package that contains the import package importlib_metadata) to requirements. So why did it ever pass? Probably because, once upon a time, another package required importlib-metadata. So it ended up in the lockfile, which you removed here. Which goes to show a benefit of not using the lockfile in CI: errors like this one (me forgetting to add importlib-metadata to pyproject.toml) didn't surface until the lockfile was removed.

Ok, so that explains the ModuleNotFoundError. What about the TypeError for unexpected keyword argument?

Subtly, the versions of entry_points() that are in importlib.metadata and importlib_metadata differ:

importlib.metadata (3.9):

entry_points()
    Return EntryPoint objects for all installed packages.

    :return: EntryPoint objects for all installed packages.

importlib_metadata (current pypi version, 7.1.0):

entry_points(**params) -> 'EntryPoints'
    Return EntryPoint objects for all installed packages.

    Pass selection parameters (group or name) to filter the
    result to entry points matching those properties (see
    EntryPoints.select()).

    :return: EntryPoints for all installed packages.

So it appears that it's loading entry_points from importlib.metadata, even though it's python 3.9 and should've imported from importlib_metadata. I'm not sure why that's happening, and I'll look into it. As for not registering any plugins, that might be an issue with your installation of derivative. Try recreating the environment and reinstalling. A fresh install on 3.9 gives me:

>>> from importlib_metadata import entry_points
>>> entry_points(group="derivative.hyperparam_opt")
EntryPoints((EntryPoint(name='kalman.default', value='derivative.utils:_default_kalman', group='derivative.hyperparam_opt'),))
>>> 
Jacob-Stevens-Haas commented 4 months ago

Oh, I see. The same commit that added importlib-metadata to pyproject.toml also removed the guard that controlled where entry_points() was imported from, resulting in the 3.9 importlib.metadata rather than the backported importlib_metadata. Reverting that (see #44) should fix the tests.

Jacob-Stevens-Haas commented 4 months ago

@andgoldschmidt Do you want a final review, or should we merge?