search

xgcm / xrft

Fourier transforms on xarray data structures
https://xrft.readthedocs.io/en/latest/
MIT License
154 stars 45 forks source link

`xrft.fft` swallows unused kwargs and does not raise a TypeError #200

Closed cgahr closed 4 months ago

cgahr commented 1 year ago

If I run

xrft.fft(data, doesnt_exist=False)

I expect an TypeError to be raised since I used an kwarg that does not exist.

The bug is caused by lines 377ff:

    if "real" in kwargs:
        real_dim = kwargs.get("real")
        warnings.warn(_real_flag_warning, FutureWarning)

There, you don't check whether kwargs is empty or not.

This bug is especially frustrating if you have a typo in a kwarg that DOES exist, like true_amplitudes instead of true_amplitude.

As far as I can tell, this only happens for xrft.fft and xrft.ifft.

TL, DR: raise a TypeError a la TypeError: fft() got an unexpected keyword argument 'doesnt_exist'

roxyboy commented 1 year ago

Thanks for raising the issue and idea for a remedy @cgahr . I'll work on it soon :)

cgahr commented 5 months ago

If you are interested, I can also have a look and submit a PR.

roxyboy commented 5 months ago

@cgahr Sorry for being slow on this. I'd appreciate a PR very much! :)

cgahr commented 5 months ago

Don't worry!

I had a quick look how easy it is to fix. When I run pytest on the current master, however, I get 4 errors and 398 warnings. Should I ignore those or fix those first or what's the best way to proceed?

roxyboy commented 5 months ago

Don't worry!

I had a quick look how easy it is to fix. When I run pytest on the current master, however, I get 4 errors and 398 warnings. Should I ignore those or fix those first or what's the best way to proceed?

Thanks @cgahr ! It's hard for me to comment on this without knowing what the errors and warnings are but errors are generally things that should be fixed. Warnings can be ignored in my opinion. Can you let me know what your Python environment is where you ran pytest?

cgahr commented 5 months ago

I used the conda env specified in the ci folder with python 3.12.4., which caused some of the problems (I think) ...

roxyboy commented 5 months ago

Ah, we should probably update the ci.yml for the Github workflow... Let me see what happens when I run the tests in python 3.12.4

cgahr commented 5 months ago

I fixed the tests locally, if you want I can add a PR to update to python 3.12

roxyboy commented 5 months ago

I fixed the tests locally, if you want I can add a PR to update to python 3.12

I've been working on a PR here: #206 . The tests also passed locally for me but I'm having issues configuring the Github workflow for Python 3.12...