-
Notifications
You must be signed in to change notification settings - Fork 422
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
Add name2taxid function from taxonkit #6146
Conversation
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please name this file .test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
help="Input the column value in which the name are written. The first column is 1 and so on. (Default: 1)" /> | ||
<param argument="--sci-name" type="boolean" falsevalue="" truevalue="--sci-name" checked="false" label="Only searching scientific names"/> | ||
<param argument="--show-rank" type="boolean" falsevalue="" truevalue="--show-rank" checked="false" label="Show rank" /> | ||
<conditional name="data"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indentation is really off here ... can you please use the https://github.com/galaxyproject/galaxy-language-server and use the autoformat option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it should be fix now but im not sure but it look better now at least at this postion where you tag it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its broken again. Please use the https://github.com/galaxyproject/galaxy-language-server plugin, this will help you a lot and also fixes all the formatting for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this add-on, but is does nothing on my side... I save the file and at this moment they should auto format it, which it didn't do. Also, it seems that the file which I uploaded always was not the file showed here with the broken format. I had the format on my side always correct, but it was shown always broken here on GitHub, which is strange. I now edited manually over GitHub in the hope that the format is now correct!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ctrl+Shift+P and then format the document and "Galaxy tool: sort the attributes of all...".
You are using TABS and should use 4-spaces. Uploading the document does not change the content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay it should be done now with 5e4af94 .
Sorry for this stupid thing!
<inputs> | ||
<param name="input" type="data" format="tabular" label="Input file" | ||
help="Input any tsv file where the NCBI names are written. You can also use a .txt but only one name per row!" /> | ||
<param argument="--name-field" type="integer" value="1" min="0" label="Column with the names" optional="false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use the data_column here https://docs.galaxyproject.org/en/latest/dev/schema.html#data-column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</conditional> | ||
<param name="sci_name" value="true"/> | ||
<param name="show_rank" value="true"/> | ||
<output name="output"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can u please compare the output generated by the tool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done with 855fae6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor changes, then good from my side
<param argument="--name-field" type="data_column" data_ref="input" label="Select column with the names" | ||
help="Select the colum where the name are written" /> | ||
<param argument="--sci-name" type="boolean" falsevalue="" truevalue="--sci-name" checked="false" label="Only searching scientific names"/> | ||
<param argument="--show-rank" type="boolean" falsevalue="" truevalue="--show-rank" checked="false" label="Show rank" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you specify what this shows ? Does it include the rank like this _g
? Maybe show a small example in the help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<data name="output" format="tabular" label="Names2taxID" /> | ||
</outputs> | ||
<tests> | ||
<test> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can u add a test for the rank option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no way i can add this since i need special lines from the original database and i dont know how they work together to get the rank option. I did add an example in the help section to show how the output should look it you use a complete database
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
The format was not boken on my side so i try to fix in here in GitHub now. I hope this is better now
... maybe now the format is fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor text changes, looks good otherwise
ln -s '$taxdump' 'taxdump.tar.gz' && | ||
tar -xf 'taxdump.tar.gz' -C '.' && | ||
#else: | ||
ln -s '$ncbi.fields.path/names.dmp' 'names.dmp' && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need delnodes.dmp and names.dmp in the test-db if we only use the test.tar.gz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, i will remove it,
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
Co-authored-by: paulzierep <paul.zierep@googlemail.com>
@paulzierep all open reviews should be done |
Good from my side @bgruening ? |
oh wait yes we need the .dmp file for the other wrapper |
Now we are good! |
<param argument="--show-rank" type="boolean" falsevalue="" truevalue="--show-rank" checked="false" label="Show rank" help="Use this option to yield the rank of the name in the output. For an example look at the help section!"/> | ||
<conditional name="data"> | ||
<param name="is_select" type="select" label="Use either a cached NCBI database or provide a downloaded version."> | ||
<option value="dm">Cached database</option> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is really just nitpicking. But since those params are exposed to users in workflows etc ... can we rename the dm
to cached
.
and his to history?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a very small nit
Done @bgruening |
Something is missing, all tests are failing atm. |
Unfortunately, not :( |
Yes I don't know why I will take a look at it later now I have to change the dm from @paulzierep a little bit ^^` |
Okay i now know it fails. The other tool which is in the same directory cause the failed test. I will have a look at this till tomorrow ! |
@bgruening okay now is good to merge. I don't know what the tools did that they got some error..... |
Great, thanks! |
Attention: deployment failure! https://github.com/galaxyproject/tools-iuc/actions/runs/10392516487 |
1 similar comment
Attention: deployment failure! https://github.com/galaxyproject/tools-iuc/actions/runs/10392516487 |
Did I do something wrong? I can, don't know how to fix it :( |
I'm looking at it. Consider it as done. |
Okay thank you! |
* Add name2taxid function from taxonkit * rename file * rename file * rename file and change a param * a little fix * change the test such that files will be compared * make the name options a bit clear * did an example for the show rank option * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * test for fix * test commit * fix test * fix test * commit to fix format problems * Update taxonkit_name2taxid.xml The format was not boken on my side so i try to fix in here in GitHub now. I hope this is better now * format fix maybe * delet file for reseting it on github * add the deleted file to see if the format is fixes now * Update taxonkit_name2taxid.xml ... maybe now the format is fixed * foramted * Update tools/taxonkit/taxonkit_name2taxid.xml * change such that the newest version will be dowanloaded and unpacked for the user * change but fomrat problem * fix file format * Apply suggestions from code review * Delete tools/taxonkit/test-data/test-db/names.dmp * Delete tools/taxonkit/test-data/test-db/delnodes.dmp * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * Update tools/taxonkit/taxonkit_name2taxid.xml Co-authored-by: paulzierep <paul.zierep@googlemail.com> * revert deleting * change value names * fix test * now fixed --------- Co-authored-by: paulzierep <paul.zierep@googlemail.com> Co-authored-by: Björn Grüning <bjoern@gruenings.eu>
FOR CONTRIBUTOR: