Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
cgahr opened this issue Jun 28, 2023 · 9 comments
Closed

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

cgahr opened this issue Jun 28, 2023 · 9 comments

Comments

@cgahr
Copy link

cgahr commented Jun 28, 2023

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
Copy link
Member

roxyboy commented Sep 20, 2023

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

@cgahr
Copy link
Author

cgahr commented Jun 30, 2024

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

@roxyboy
Copy link
Member

roxyboy commented Jun 30, 2024

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

@cgahr
Copy link
Author

cgahr commented Jul 1, 2024

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
Copy link
Member

roxyboy commented Jul 1, 2024

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
Copy link
Author

cgahr commented Jul 1, 2024

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

@roxyboy
Copy link
Member

roxyboy commented Jul 1, 2024

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
Copy link
Author

cgahr commented Jul 1, 2024

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

@roxyboy
Copy link
Member

roxyboy commented Jul 1, 2024

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...

@roxyboy roxyboy closed this as completed in caae000 Jul 3, 2024
roxyboy added a commit that referenced this issue Jul 3, 2024
fix #200: raise TypeError on unused kwargs
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants