Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

FIX: Deprecation of inspect.getfullargspec (#99) #100

Merged
merged 1 commit into from Jul 27, 2023
Merged

FIX: Deprecation of inspect.getfullargspec (#99) #100

merged 1 commit into from Jul 27, 2023

Conversation

kotopesutility
Copy link
Contributor

inspect.getfullargspec is deprecated (see https://docs.python.org/3/library/inspect.html#inspect.getfullargspec) and does not support numpy_linalg.eigh for numpy >= 1.25 (see #99)

@Stewori
Copy link
Member

Stewori commented Jul 25, 2023

Thank you for this PR -- the deprecation should indeed be fixed. However, I doubt that it is sufficient to fix #99 but did not check. Did you test it with recent NumPy? I suspect, inspect will still raise TypeError: unsupported callable. I guess, https://github.com/mdp-toolkit/mdp-toolkit/blob/master/mdp/configuration.py#L338 needs to be fixed, e.g.\ by wrapping in try/except TypeError.

@kotopesutility
Copy link
Contributor Author

Thank you for this PR -- the deprecation should indeed be fixed.

Thank you for your reply!

However, I doubt that it is sufficient to fix #99 but did not check. Did you test it with recent NumPy?

Yes

I suspect, inspect will still raise TypeError: unsupported callable.

Well, I checked once again and my solution works. Also all tests passed.

I guess, https://github.com/mdp-toolkit/mdp-toolkit/blob/master/mdp/configuration.py#L338 needs to be fixed, e.g.\ by wrapping in try/except TypeError.

If you get TypeError, you still need to check if it is numpy.

I don't like that I use a tuple with None at the end, but what I did was to patch just one line.

@Stewori
Copy link
Member

Stewori commented Jul 25, 2023

If you get TypeError, you still need to check if it is numpy.

Yes, that's not ideal, your fix is better if it is sufficient as you say.

@Stewori
Copy link
Member

Stewori commented Jul 26, 2023

Thanks again for this PR. Given that tests pass I am going to merge it soon. After having a look at the linked doc, I conclude that getfullargspec is not actually deprecated (as of Python 3.11). It was deprecated but the decision has been reversed. Still, its use is not recommended and migrating to signature is the way to go. However, calling it "deprecated" in the comment is misleading, so I kindly ask you to change the comment, e.g. to "Migrate to signature as recommended in" (link next line). I am still surprised that it fixes #99 because the linked doc states that getfullargspec is already based on signature since Python 3.4.
Please feel free to add yourself to the contributors list https://github.com/mdp-toolkit/mdp-toolkit.github.io/blob/master/development.html by making a PR in https://github.com/mdp-toolkit/mdp-toolkit.github.io. (Note that the list is ordered alphabetically by last names)

@kotopesutility
Copy link
Contributor Author

However, calling it "deprecated" in the comment is misleading, so I kindly ask you to change the comment, e.g. to "Migrate to signature as recommended in" (link next line).

Done, I have force-pushed with new comment. Please check it!

@Stewori Stewori merged commit 5a7d765 into mdp-toolkit:master Jul 27, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants