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 gaussian import from scipy #2332

Merged
merged 1 commit into from
Mar 25, 2024
Merged

Conversation

OriolAbril
Copy link
Member

@OriolAbril OriolAbril commented Mar 25, 2024

Description

closes #2330

Checklist

  • Does the PR follow official
    PR format?
  • Does the PR include new or updated tests to prevent issue recurrence (using pytest fixture pattern)?
  • Is the code style correct (follows pylint and black guidelines)?
  • Is the fix listed in the Maintenance and fixes
    section of the changelog?

📚 Documentation preview 📚: https://arviz--2332.org.readthedocs.build/en/2332/

@tomicapretto
Copy link
Contributor

Is this related to any specific version of SciPy or any detail that could be made explicit? It's just curiosity, the change looks good.

@OriolAbril
Copy link
Member Author

The function has been moved, starting with next release trying to import it from scipy.signal instead of scipy.signal.windows will raise an error. The issue this PR will close is because this is already happening with the latest scipy pre-release

@OriolAbril OriolAbril merged commit 9d66eaa into arviz-devs:main Mar 25, 2024
10 checks passed
@OriolAbril OriolAbril deleted the fix_gaussian branch March 25, 2024 22:26
@juanitorduz
Copy link

Hey! as the scipy 1.13 was released yesterday (https://pypi.org/project/scipy/1.13.0/) Will you cut a release with this fix soon 🙏 ?

@zerothi
Copy link

zerothi commented Sep 11, 2024

Next time things like this happens, it would be more ideal to do something like:

try:
    # import-change since version 1.13!
    from scipy.signal.windows import gaussian
except:
    from scipy.signal import gaussian

in this way you are backwards-compatible without these cascading problems.

(even when things like this was actually a mistake, it seems that gaussian was always supposed to be imported from the windows subpackage.)

@OriolAbril
Copy link
Member Author

OriolAbril commented Sep 11, 2024

We generally do that @zerothi. However in that case the issue is we missed the DeprecationWarning, so the issue and PR were open after the deprecation process, when scipy.signal.gaussian was finally removed. IIRC, scipy.signal.gaussian was removed in scipy 1.13, but scipy.signal.windows.gaussian has been around since at least scipy 1.7, and ArviZ requires scipy >=1.9. There is no version of scipy supported by ArviZ where from scipy.signal.windows import gaussian should fail.

Keep in mind the original code was probably written around scipy 1.0 at which time the scipy.signal.windows submodule didn't exist yet: https://docs.scipy.org/doc/scipy-1.0.0/reference/signal.html#window-functions and it has probably changed locations within ArviZ but mostly copy pasting to restructure imports.

@zerothi
Copy link

zerothi commented Sep 11, 2024

Ok, I agree with your comments! Thanks!!

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.

arviz not compatible with pre-releases
4 participants