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.

Sign up for GitHub

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

Jump to bottom

Feature/add met frag #2098

Closed
wants to merge 7 commits into from
Closed

Feature/add met frag #2098

wants to merge 7 commits into from

Conversation

sneumann
Copy link
Contributor

@sneumann sneumann commented Sep 21, 2018 •
edited
Loading

FOR CONTRIBUTOR:

  • - I have read the CONTRIBUTING.md document and this tool is appropriate for the tools-iuc repo.
  • - License permits unrestricted use (educational + commercial)
  • - This PR adds a new tool or tool collection
  • - This PR updates an existing tool or tool collection
  • - This PR does something else (explain below)

FOR REVIEWER:

  • .shed.yml file ok
    • Toolshed user iuc has access to associated toolshed repo(s)
  • Indentation is correct (4 spaces)
  • Tool version/build ok
  • <command/>
    • Text parameters, input and output files 'single quoted'
    • Use of <![CDATA[ ... ]]> tags
    • Parameters of type text or having optional="true" attribute are checked with if str($param) before being used
  • Data parameters have a format attribute containing datatypes recognised by Galaxy
  • Tests
    • Parameters are reasonably covered
    • Test files are appropriate
  • Help
    • Valid restructuredText and uses <![CDATA[ ... ]]> tags
  • Complies with other best practice in Best Practices Doc

bgruening
bgruening reviewed Sep 23, 2018
View reviewed changes
Copy link
Member

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Thanks @sneumann!
I added a few comments. Please also consider to use tab-separated-files as Galaxy has a bunch of tools to process them without converting them in between.

tools/metfrag/metfrag-cli-batch.xml
@@ -0,0 +1,149 @@
<?xml version='1.0' encoding='UTF-8'?>
<!--This is a configuration file for the integration of a tools into Galaxy (https://galaxyproject.org/).-->
Copy link
Member

Choose a reason for hiding this comment

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

can we remove these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gone.

tools/metfrag/metfrag-cli-batch.xml
<?xml version='1.0' encoding='UTF-8'?>
<!--This is a configuration file for the integration of a tools into Galaxy (https://galaxyproject.org/).-->
<!--Proposed Tool Section: [PhenoMeNal]-->
<tool id="metfrag-cli-batch" name="metfrag-cli-batch" version="0.4">
Copy link
Member

Choose a reason for hiding this comment

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

Can we use _ instead of - for the id?
I would also use Run MetFrag as name and the rest as a description.

tools/metfrag/metfrag-cli-batch.xml
<command detect_errors="aggressive"><![CDATA[
metfrag ParameterFile='$metfrag_cli_batch_input_1' ResultsFile='$metfrag_cli_batch_output_1'
#if $metfrag_cli_batch_database_file != "None"
LocalDatabasePath="$metfrag_cli_batch_database_file"
Copy link
Member

Choose a reason for hiding this comment

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

indentation and single quotes :)

tools/metfrag/metfrag-cli-batch.xml
</command>
<inputs>
<param name="metfrag_cli_batch_input_1" type="data" format="txt" optional="False" label="File with MetFrag parameters" help="File containing MetFrag parameters which is generated by msms2metfrag" />
<param name="additionalparameters" type="text" value="" optional="True" label="Additional Parameters" help="Additional parameters that can be passed to MetFrag. This can include e.g. access information for a local MetChem database instance" />
Copy link
Member

Choose a reason for hiding this comment

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

This can be very dangerous, as a user can pass everything to the command line. Are there too many parameters to explicitly list them here?

tools/metfrag/metfrag-cli-batch.xml
<param name="metfrag_cli_batch_input_1" type="data" format="txt" optional="False" label="File with MetFrag parameters" help="File containing MetFrag parameters which is generated by msms2metfrag" />
<param name="additionalparameters" type="text" value="" optional="True" label="Additional Parameters" help="Additional parameters that can be passed to MetFrag. This can include e.g. access information for a local MetChem database instance" />
<param name="metfrag_cli_batch_database_file" type="data" format="csv" optional="True" label="Database File" help="File containing candidates used as database" />
<param name="additional_metfrag_scores" type="select" display="checkboxes" multiple="true" label="Additional MetFrag scores" help="Add additional MetFrag scores used to score candidates">
Copy link
Member

Choose a reason for hiding this comment

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

a select with only one option looks wired, do you want to use a boolean here?

tools/metfrag/metfrag-cli-batch.xml
| A csv output file with scored candidates and properties
|

---------------------------------------------------
Copy link
Member

Choose a reason for hiding this comment

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

We do not list news or changes in a tool. This can be done in an additional readme file if you like.

tools/metfrag/metfrag-cli-batch.xml
| 2) Database File (optional) | csv |
+------------------------------+------------+

----------
Copy link
Member

Choose a reason for hiding this comment

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

These parameter description does not add more information as the above help text. It would be nice if we can add additional information here.

@bernt-matthias
Copy link
Contributor

@sneumann still needed in IUC? Seems that there are new recent plans: computational-metabolomics/metfrag-galaxy#9

@bernt-matthias
Copy link
Contributor

I think we can close this now since metfrag is now in Galaxy https://github.com/computational-metabolomics/metfrag-galaxy. Or?

@sneumann
Copy link
Contributor Author

sneumann commented Feb 8, 2021

Yes, happy to see it maintained as Galaxy tool. Thanks Ralf et al ! Yours, Steffen

@sneumann sneumann closed this Feb 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgruening bgruening bgruening left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sneumann @bernt-matthias @bgruening