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

Moving karyotype to anoph #702

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Moving karyotype to anoph #702

wants to merge 6 commits into from

Conversation

jonbrenas
Copy link
Collaborator

Addresses #698.

The move is done and all tests pass locally but more tests need to be added (for funestus, for example).

@jonbrenas jonbrenas marked this pull request as draft December 11, 2024 14:52
@jonbrenas
Copy link
Collaborator Author

I added tests to test_ag3.py and test_af1.py to check that errors were indeed raised when expected. I am not really satisfied yet, though.

@jonbrenas jonbrenas marked this pull request as ready for review December 11, 2024 16:51
Copy link
Member

@alimanfoo alimanfoo left a comment

Choose a reason for hiding this comment

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

Hi @jonbrenas, thanks for this, a few suggestions...

malariagen_data/anoph/karyotype.py Outdated Show resolved Hide resolved
malariagen_data/anoph/karyotype.py Outdated Show resolved Hide resolved
malariagen_data/anoph/karyotype.py Outdated Show resolved Hide resolved
malariagen_data/anoph/karyotype.py Outdated Show resolved Hide resolved
Comment on lines +39 to +42
if not self._inversion_tag_path:
raise FileNotFoundError(
"The file containing the inversion tags is missing."
)
Copy link
Member

Choose a reason for hiding this comment

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

FileNotFoundError isn't quite the right exception class here.

Suggested change
if not self._inversion_tag_path:
raise FileNotFoundError(
"The file containing the inversion tags is missing."
)
if self._inversion_tag_path is None:
raise NotImplementedError(
"No inversion tags are available for this data resource."
)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have been of two minds about this one: on one hand, it is true that we have not generated the tags for Af1 and one could argue that NotImplementedError is a more appropriate error for work that is still to be done; on the other hand, the code itself would (I think) work if the file actually existed and it is thus less an issue of missing code and more an issue of a missing input. Also, I can imagine a situation where someone created tags for an inversion and would want to try to use their local file (it is not currently possible, the path that is used is hard-coded in both Ag3 and Af1) in which case the error would come from the path being incorrect (though, an error message referring to the actual path inputed would be more helpful if we want to offer this option).

malariagen_data/anoph/karyotype.py Show resolved Hide resolved
malariagen_data/anoph/karyotype_params.py Outdated Show resolved Hide resolved
Comment on lines 87 to 98
if inversion == "X_x":
with pytest.raises(TypeError):
af1.karyotype(
inversion=inversion, sample_sets="AG1000G-GH", sample_query=None
)
else:
with pytest.raises(FileNotFoundError):
af1.karyotype(
inversion=inversion,
sample_sets="1229-VO-GH-DADZIE-VMF00095",
sample_query=None,
)
Copy link
Member

Choose a reason for hiding this comment

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

All inversion parameter values should fail with NotImplementedError I think.

tests/integration/test_ag3.py Outdated Show resolved Hide resolved
@alimanfoo
Copy link
Member

The other question here is if/how to get unit tests using the simulated data. It could be tricky because the simulated data is not guaranteed to generate data at the tag SNP positions.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jonbrenas
Copy link
Collaborator Author

Thanks @alimanfoo. I made most of the changes you recommended. As for the tests, I created another issue #700 to deal with it independently of this as I agree that it is not trivial.

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.

2 participants