fix gaussian import from scipy

arviz-devs / arviz

Exploratory analysis of Bayesian models with Python

https://python.arviz.org

Apache License 2.0

1.59k stars 395 forks source link

fix gaussian import from scipy #2332

Closed OriolAbril closed 6 months ago

OriolAbril commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

juanitorduz commented 6 months ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Ok, I agree with your comments! Thanks!!