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

Make biopython restriction open-ended #28

Merged
merged 3 commits into from
Dec 20, 2022
Merged

Make biopython restriction open-ended #28

merged 3 commits into from
Dec 20, 2022

Conversation

ecederstrand
Copy link
Contributor

This matches what requirements.txt already has. Unpinning the dependency makes it easier to combine bio with other packages that require biopython

This matches what requirements.txt already has. Unpinning the dependency makes it easier to combine `bio` with other packages that require `biopython`
@ialbert
Copy link
Owner

ialbert commented Dec 20, 2022

Unfortunately, I must pin the version because biopython introduced a functionality-breaking change in 1.80. There is a workaround as described here:

biopython/biopython#4183

but I have not yet gotten to implement and test that change in bio

@ecederstrand
Copy link
Contributor Author

Do you have a test case that fails with biopython==1.80? If so, I may be able to find time to contribute a patch.

@ialbert
Copy link
Owner

ialbert commented Dec 20, 2022

This is the line that fails:

https://github.com/ialbert/bio/blob/master/biorun/align.py#L210

It expects the aligned sequences and a trace.

I was told by @Paradoxdruid that the following underscore function would work for 1.80:

 aln._format_unicode()

but I have not yet tested it. It might work for 1.79 as well.

It is an underscore method, and that indicates that its function may change in the future.

@Paradoxdruid
Copy link

Just chiming in; although I dislike breaking changes, the new behavior actually considerably simplified my code:

First, I'm determining biopython version via

from Bio import __version__ as bio_version
bio_minor: int = int(bio_version.split(".")[1])

Then, my codepaths: https://github.com/Paradoxdruid/pyllelic/blob/master/pyllelic/quma.py#L390-L412

That much shorter 1.80 implementation still passes all the unit tests and passes manual spot checking.

All that to say; it might be worth writing to require biopython >= 1.80 to use the new behavior.

@ecederstrand
Copy link
Contributor Author

I'm quite unfamiliar with this package and testing changes. I've added some commits to this PR. Is this what you had in mind?

@ialbert
Copy link
Owner

ialbert commented Dec 20, 2022

Ah splendid fix. Looks like the code had some remnants of a previous implementation that were not even needed. Thanks for tracking that down.

The code will be merged shortly and updated on PyPI

Looks like all tests pass. You can do

bio test

at any time to test the package, though occasionally the remote data changes and then of course those tests can will fail

(in those cases the tool shows both a diff to the results and the command one would need to be run to recreate the local test data that failed)

@ialbert ialbert merged commit a746b54 into ialbert:master Dec 20, 2022
@ecederstrand ecederstrand deleted the patch-1 branch December 20, 2022 20:51
This pull request was closed.
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.

3 participants