Skip to content

Amin1#75

Merged
alimanfoo merged 6 commits intomalariagen:masterfrom
alimanfoo:amin1-alimanfoo-2021-11-03
Nov 9, 2021
Merged

Amin1#75
alimanfoo merged 6 commits intomalariagen:masterfrom
alimanfoo:amin1-alimanfoo-2021-11-03

Conversation

@alimanfoo
Copy link
Copy Markdown
Member

@alimanfoo alimanfoo commented Nov 4, 2021 •
edited
Loading

In this PR:

  • Add an Amin1 class for accessing data from the minimus v1 release.
  • Add support for providing a list of contigs via the contig parameter in both Amin1 and Ag3.
  • Some improvements to tests and refactoring of common code between Amin1 and Ag3.

@alimanfoo alimanfoo requested a review from cclarkson November 4, 2021 23:32
cclarkson
cclarkson approved these changes Nov 8, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@cclarkson cclarkson left a comment

Choose a reason for hiding this comment

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

Apart from doc string update, it all looks good to me.

I did get 1 warning when running amin1 tests on pycharm, it would be good to run all the amin methods in a notebook once released to check.

@cclarkson
Copy link
Copy Markdown
Collaborator

cclarkson commented Nov 8, 2021

Out of interest, does the new fsspec setup give us anything extra, or is it just API changes?

@alimanfoo
Copy link
Copy Markdown
Member Author

alimanfoo commented Nov 9, 2021

Out of interest, does the new fsspec setup give us anything extra, or is it just API changes?

Do you mean the init_filesystem() function in the malariagen_data.util module? If so, that's just refactoring, moving out code needed by both Ag3 and Amin1 classes to a common location.

@alimanfoo
Copy link
Copy Markdown
Member Author

alimanfoo commented Nov 9, 2021

I did get 1 warning when running amin1 tests on pycharm, it would be good to run all the amin methods in a notebook once released to check.

I ran the tests from the terminal with an assert False in there to get the warnings reported, it looks fairly benign, not going to worry about for now.

@alimanfoo
Copy link
Copy Markdown
Member Author

alimanfoo commented Nov 9, 2021

Thanks again @cclarkson for the review. I'll merge if CI passes.

@alimanfoo alimanfoo merged commit fc85c6f into malariagen:master Nov 9, 2021
@alimanfoo alimanfoo deleted the amin1-alimanfoo-2021-11-03 branch November 9, 2021 12:37
@alimanfoo alimanfoo added the BMGF-001927 Work supported by BMGF grant INV-001927 (MalariaGEN 2019-2024). label Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cclarkson cclarkson cclarkson approved these changes

Assignees

No one assigned

Labels

BMGF-001927 Work supported by BMGF grant INV-001927 (MalariaGEN 2019-2024).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alimanfoo @cclarkson