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 binom_test in scipy 1.12.0 and cryptic error occurring with incorrect VCFs in cyvcf2 0.30.26 #208

Merged
merged 5 commits into from
Jan 27, 2024

Conversation

aryarm
Copy link
Member

@aryarm aryarm commented Jan 23, 2024

This PR handles errors that arise when you upgrade to the newest versions of our dependencies, specifically for cyvcf2 and scipy

It resolves #206 and also handles the deprecation of scipy.stats.binom_test in favor of scipy.stats.binomtest: https://docs.scipy.org/doc/scipy/release/1.12.0-notes.html#expired-deprecations

Merging this PR will resolve failures in the CI checks for all of our other PRs

@aryarm aryarm marked this pull request as ready for review January 24, 2024 00:49
@aryarm
Copy link
Member Author

aryarm commented Jan 24, 2024

@LiterallyUniqueLogin, I decided to catch the htslib/cyvcf2 error in all downstream tools that used TRRecordHarmonizer (which was actually just dumpSTR and qcSTR?), since it seemed easy enough

Let me know if there are any other changes we need!

@aryarm aryarm changed the title fix: handle deprecation of binom_test in scipy 1.12.0 and clarify cryptic error occurring with incorrect VCFs in cyvcf2 0.30.26 fix: deprecation of binom_test in scipy 1.12.0 and cryptic error occurring with incorrect VCFs in cyvcf2 0.30.26 Jan 24, 2024
@aryarm
Copy link
Member Author

aryarm commented Jan 26, 2024

@LiterallyUniqueLogin, I'm realizing now that this error will probably crop up in any tool that uses cyvcf2, and not just in dumpSTR and qcSTR.

The proper way to handle this is probably to add an equivalent of the BrokenVCF test to every tool, and then use those tests to figure out how to properly catch the exceptions. But tbh those sorts of changes would probably bring this PR beyond what I have the bandwidth for. Also, I kinda doubt that I'm familiar enough with TRTools to be able to write tests like that.

@LiterallyUniqueLogin
Copy link
Contributor

Your reasoning sounds good. Fixing this shouldn't be held up by missing tests for other utilities.

@LiterallyUniqueLogin LiterallyUniqueLogin merged commit 78ec86a into master Jan 27, 2024
7 checks passed
@LiterallyUniqueLogin LiterallyUniqueLogin deleted the fix/deprecations branch January 27, 2024 22:07
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.

broken test test_dumpSTR.py::test_BrokenVCF
2 participants