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

ENH: New type called AMRFinderPlusDatabase #81

Merged
merged 13 commits into from
Jul 11, 2024

Conversation

VinzentRisch
Copy link
Collaborator

@VinzentRisch VinzentRisch commented Jul 1, 2024

solves #80

This new type is used for the storage of the database for the amrfinderplus tool.

Set up an environment

https://github.com/bokulich-lab/q2-amr?tab=readme-ov-file#installation

Run it locally

  1. First, clone the repo and checkout the PR branch:
git clone https://github.com/bokulich-lab/q2-amr.git
cd q2-amr
git fetch origin pull/81/head:pr-81
git checkout pr-81
pip install -e .
  1. Download test files
    amrfinderplus_database.zip: https://polybox.ethz.ch/index.php/s/nJmbGpkM1ywS9VW

  2. Test it out!

qiime tools import --input-path database --output-path database.qza --type AMRFinderPlusDatabase 

@VinzentRisch
Copy link
Collaborator Author

Hey @misialq
Could you have a look at this? I now created two formats that just validate any binary or text file. Should I add formats for the tab separated files?
I could add new formats for these files that just validates the header:
AMR_DNA-Streptococcus_pneumoniae.tab
changes.txt
database_format_version.txt
fam.tab
taxgroup.tab
version.txt
AMRProt-mutation.tab
AMRProt-susceptible.tab

Those would be 8 new formats so Im not so sure if that even makes sense.

@VinzentRisch VinzentRisch requested a review from misialq July 2, 2024 10:01
q2_amr/amrfinderplus/types/_format.py Outdated Show resolved Hide resolved
q2_amr/amrfinderplus/types/_transformer.py Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Once you change the directory format as I pointed out above, you can remove most of those test files - they are just polluting the repo.

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 left them there, because if I remove most files, the test would become meaningless.
Only like this it is actually tested if all regex expressions work with the database files.

Copy link
Contributor

@misialq misialq Jul 11, 2024

Choose a reason for hiding this comment

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

Why do you think it will become meaningless? Your test should just make sure that the regex is working - for this you don't need all the files for all the species - you could just keep a set of those for, say, two taxa and remove everything else (now that you have the FileCollection in place).

@VinzentRisch VinzentRisch requested review from misialq July 4, 2024 10:22
@VinzentRisch
Copy link
Collaborator Author

Hi @misialq could you have another look at this?
I'm not so sure about the path makers. Do they work like this with a regex?

@VinzentRisch VinzentRisch requested a review from misialq July 11, 2024 10:03
Copy link
Contributor

@misialq misialq left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

@misialq misialq merged commit fb9e4b5 into bokulich-lab:main Jul 11, 2024
2 checks passed
VinzentRisch added a commit to VinzentRisch/q2-rgi that referenced this pull request Sep 27, 2024
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