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

MAINT: avoid call to deprecated scipy.special.btdtr and scipy.special.btdtri functions #884

Merged
merged 3 commits into from
Jan 9, 2025

Conversation

mj-will
Copy link
Collaborator

@mj-will mj-will commented Jan 7, 2025

scipy.special.btdtr and scipy.special.btdtri are deprecated and were removed in scipy 1.14.0 (see the docs here and here). The PR replaces the import and call with betainc and betaincinv which are equivalent.

Motivation

Since this function has been removed, we have to change the code if we are going to support scipy >= 1.14.0.

The CI was failing due to this import. See e.g. https://github.com/bilby-dev/bilby/actions/runs/12650850335

Testing

I've run the tests locally and they pass.

Other notes

I also reformatted the imports to follow the formatting used by black

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 7, 2025

I noticed whilst making this change that we're importing functions from scipy.special._ufuncs rather than scipy.special. Does anyone know why that is?

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 7, 2025

It seems there may be another change in, I think, matplotlib that is breaking a different part of the test suite. I'll have a look and see if I can fix it as a part of this PR as well.

Copy link
Collaborator

@GregoryAshton GregoryAshton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me

@GregoryAshton
Copy link
Collaborator

I noticed whilst making this change that we're importing functions from scipy.special._ufuncs rather than scipy.special. Does anyone know why that is?

I think that the functions may not have existed in scipy.special at first, but only in ufuncs (at least this is my recollection).

@ColmTalbot
Copy link
Collaborator

It seems there may be another change in, I think, matplotlib that is breaking a different part of the test suite. I'll have a look and see if I can fix it as a part of this PR as well.

This is due to an incompatibility between gwpy and matplotlib, that Duncan said will be resolved by a future release of gwpy.

@ColmTalbot
Copy link
Collaborator

I noticed whilst making this change that we're importing functions from scipy.special._ufuncs rather than scipy.special. Does anyone know why that is?

I don't know, but I'd be in favour of changing it, especially with array api support coming soon for non _ufunc imports. The only thing I can think of is that there is a performance improvement to using this.

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 7, 2025

Thanks @GregoryAshton and @ColmTalbot, given that I'll change the code to just import from .special directly.

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 7, 2025

It seems there may be another change in, I think, matplotlib that is breaking a different part of the test suite. I'll have a look and see if I can fix it as a part of this PR as well.

This is due to an incompatibility between gwpy and matplotlib, that Duncan said will be resolved by a future release of gwpy.

Ah, I see, any thoughts on how to best avoid this? To me, the easiest option would probably be to just set matplotlib <3.10 in the CI.

@mj-will mj-will added this to the 2.5.0 milestone Jan 7, 2025
@ColmTalbot
Copy link
Collaborator

Ah, I see, any thoughts on how to best avoid this? To me, the easiest option would probably be to just set matplotlib <3.10 in the CI.

As a temporary workaround, sure, can we do it in a separate PR so it is easier to see/revert?

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 7, 2025

As a temporary workaround, sure, can we do it in a separate PR so it is easier to see/revert?

Sure, here's the PR: #885

@mj-will mj-will linked an issue Jan 7, 2025 that may be closed by this pull request
@mj-will mj-will force-pushed the fix-btdtri-import branch from 3dbdb54 to df581f1 Compare January 7, 2025 16:31
@mj-will
Copy link
Collaborator Author

mj-will commented Jan 8, 2025

I'm unsure why the tests are still failing. I've created a local env with matploltib 3.10 and then downgraded to 3.9.4 and the tests then pass. Does anyone have any ideas?

@mj-will
Copy link
Collaborator Author

mj-will commented Jan 8, 2025

I'm unsure why the tests are still failing. I've created a local env with matploltib 3.10 and then downgraded to 3.9.4 and the tests then pass. Does anyone have any ideas?

I think I've identified the issue and I'm trying to address it in #888

@mj-will mj-will force-pushed the fix-btdtri-import branch from df581f1 to 01dc692 Compare January 9, 2025 08:42
@mj-will mj-will changed the title MAINT: avoid call to deprecated scipy.special.btdtri function MAINT: avoid call to deprecated scipy.special.btdtr and scipy.special.btdtri functions Jan 9, 2025
@mj-will
Copy link
Collaborator Author

mj-will commented Jan 9, 2025

After rebasing, the relevant tests now run and pass. However, an unrelated test is failing. I've opened a separate PR with the fix (#891) but I can also pull the changes into this one if we want.

@adivijaykumar
Copy link
Collaborator

I'd personally prefer if changes in separate PRs stay separate! Hopefully no additional PRs will be needed.

@mj-will mj-will force-pushed the fix-btdtri-import branch from 01dc692 to 4e58bc4 Compare January 9, 2025 16:00
@mj-will mj-will merged commit 391d359 into bilby-dev:main Jan 9, 2025
11 of 12 checks passed
@mj-will mj-will deleted the fix-btdtri-import branch January 9, 2025 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatible with scipy 1.15.0
4 participants