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

Metams #275

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

Metams #275

wants to merge 206 commits into from

Conversation

lecorguille
Copy link
Member

This MR merge the metams repository https://github.com/workflow4metabolomics/metaMS/tree/prep_migration in this main one.

yguitton and others added 30 commits May 22, 2016 21:18
C'est un peu trompeur sur la marchandise tout ce vert :P
Mais pas de soucis pour y travailler avec toi.
add gg format for RI file
add txt format for msp file
Add Metabolomics tag in Galaxy toolshed descriptions.
from ticket otrs 2016083010000283 if no peaks in a samples then plotUnknown break
avoid empty dataMatrix when file names contains space or start with numbers
@jsaintvanne
Copy link
Member

jsaintvanne commented Oct 7, 2024

@bgruening
Copy link
Contributor

Third error is that we have files more than 1Mo for tests... Should we review all our test ? => check if need these files

It would be awesome to have as small test data as we can. I knows its time consuming, but its worth the effort IMHO. git repos can not be that big and we run tests very often. E.g. on IUC we run all tests of all tools with every Galaxy release, which we could also do here.

That all said, we do have official support nowadays to specify a URL to a test file, so files can be larger if necessary.

Comment on lines 1 to 2
<?xml version="1.0"?>

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<?xml version="1.0"?>

Comment on lines 7 to 11
<xml name="stdio">
<stdio>
<exit_code range="1" level="fatal" />
</stdio>
</xml>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<xml name="stdio">
<stdio>
<exit_code range="1" level="fatal" />
</stdio>
</xml>

Comment on lines 91 to 97
**Please cites**

metaMS : Wehrens, R.; Weingart, G.; Mattivi, F. Journal of Chromatography B.

xcms : Smith, C. A.; Want, E. J.; O’Maille, G.; Abagyan, R.; Siuzdak, G. Anal. Chem. 2006, 78, 779–787.

CAMERA : Kuhl, C.; Tautenhahn, R.; Böttcher, C.; Larson, T. R.; Neumann, S. Analytical Chemistry 2012, 84, 283–289.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Please cites**
metaMS : Wehrens, R.; Weingart, G.; Mattivi, F. Journal of Chromatography B.
xcms : Smith, C. A.; Want, E. J.; O’Maille, G.; Abagyan, R.; Siuzdak, G. Anal. Chem. 2006, 78, 779–787.
CAMERA : Kuhl, C.; Tautenhahn, R.; Böttcher, C.; Larson, T. R.; Neumann, S. Analytical Chemistry 2012, 84, 283–289.
**Please cites**
metaMS : Wehrens, R.; Weingart, G.; Mattivi, F. Journal of Chromatography B.
xcms : Smith, C. A.; Want, E. J.; O’Maille, G.; Abagyan, R.; Siuzdak, G. Anal. Chem. 2006, 78, 779–787.
CAMERA : Kuhl, C.; Tautenhahn, R.; Böttcher, C.; Larson, T. R.; Neumann, S. Analytical Chemistry 2012, 84, 283–289.

Is this still needed? We have the proper annotation already.


.. class:: infomark

**Galaxy wrapper and scripts developpers** Guitton Yann LABERCA [email protected] and Saint-Vanne Julien [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

</xml>

<!-- COMMAND -->
<token name="@COMMAND_RSCRIPT@">LC_ALL=C Rscript $__tool_directory__/</token>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to quote this path

@@ -0,0 +1,344 @@
<tool id="metams_runGC" name="metaMS.runGC" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<tool id="metams_runGC" name="metaMS.runGC" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@">
<tool id="metams_runGC" name="metaMS.runGC" version="@TOOL_VERSION@+galaxy@VERSION_SUFFIX@" profile="23.0">

</macros>

<expand macro="requirements"/>
<expand macro="stdio"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<expand macro="stdio"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

instead add the detect_errors tag into the command, like below

<expand macro="requirements"/>
<expand macro="stdio"/>

<command><![CDATA[
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<command><![CDATA[
<command detect_errors="exit_code"><![CDATA[

@hechth
Copy link
Contributor

hechth commented Oct 11, 2024

That all said, we do have official support nowadays to specify a URL to a test file, so files can be larger if necessary.

How can this actually be specified? Is there a tutorial for this? This would also be greatly helpful for our tools.

@bernt-matthias
Copy link
Contributor

https://docs.galaxyproject.org/en/master/dev/schema.html#tool-tests-test-param

<param name="PARAM_NAME" location="STABLE_URL"/>

@hechth
Copy link
Contributor

hechth commented Oct 11, 2024

https://docs.galaxyproject.org/en/master/dev/schema.html#tool-tests-test-param

<param name="PARAM_NAME" location="STABLE_URL"/>

Amazing, thanks!

@DamienCode404
Copy link
Contributor

To indent the code in the R files to validate the lintr step, I used the "styler" library from RStudio. The code formatting worked well.

Copy link
Contributor

@bernt-matthias bernt-matthias left a comment

Choose a reason for hiding this comment

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

Thanks for the efforts. styler is great. Recent versions of the CI also use it instead of / in addition to lintr: https://github.com/galaxyproject/tools-iuc/tree/main/.github (I will try to update CI here).

Two more small comments included.

@@ -0,0 +1,6 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This way of defining a suite is outdated. Use suite and auto_tool_repositories in shed.yaml like here https://github.com/galaxyproject/tools-iuc/blob/1019bf0fda897582e2bbdc773aebb3e08e285aae/tools/ampvis2/.shed.yml#L14

If there are different owners this might be a solution: https://github.com/galaxyproject/tools-iuc/blob/main/tool_collections/samtools/.shed.yml

<param name="EIC" value="true"/>
<param name="unkn" value="1:5"/>
</conditional>
<output name="unknownPdf" value="GCMS_EIC_noDB.pdf" ftype="pdf" compare="sim_size" delta="1200"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of sim_size you could use a <has_size size="xyz" delta="1200"/> content assertion. Then we do not need to save the actual file in the repo.

</required_files>

<command detect_errors="exit_code"><![CDATA[
@COMMAND_RSCRIPT@/metams_plot.r
Copy link
Contributor

Choose a reason for hiding this comment

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

The $__tool_directory__/metams_plot.r should be single quoted.

<option value="true">YES</option>
</param>
<when value="true">
<param name="unkn" type="text" value="1:5" label="EIC_Unknown" help="vector of peaks number to be plotted, for example 1:5 (mean 1 to 5) or 1,4,12 (means 1 4 and 12). For all EIC use 0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally this would have a validator checking for correct user input.

</data>
<!-- Only when user selected EIC option -->
<data name="unknownPdf" format="pdf" from_work_dir="GCMS_EIC.pdf" label="GCMS_EIC.pdf">
<filter>selectedeic['EIC'] is True</filter>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably selectedeic['EIC'] == "true"

<data name="variableMetadata" format="tabular" from_work_dir="variableMetadata.tsv" label="variableMetadata.tsv" />
<data name="dataMatrix" format="tabular" from_work_dir="dataMatrix.tsv" label="dataMatrix.tsv" />
<data name="peakspectra" format="txt" from_work_dir="peakspectra.msp" label="peakspectra.msp" />
<data name="rungcRData" format="rdata" from_work_dir="runGC.RData" label="runGC.RData" />
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be preferable to use format="rds" for the output. In the R-script you might just switch to saveRDS and readRDS. But since you explicitly list the stored objects in the save call also Rdata might be acceptable. Some people have security concerns with rdata.

https://www.matheusmello.io/posts/r-what-are-the-main-differences-between-r-data-files

<output name="bpcsRawPdf" value="BPCs_raw.pdf" ftype="pdf" compare="sim_size" delta="2000"/>
</test>
<!-- Test EICs -->
<test expect_num_outputs="5">
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand how you get to 5 outputs here.

<param name="metaMS" value="runGC_test.RData" ftype="rdata" />
<expand macro="test_file_load_alg_symlink" />
<param name="selectedtic" value="true"/>
<output name="ticsRawPdf" value="TICs_raw.pdf" ftype="pdf" compare="sim_size" delta="600"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of saving the file for comparison and having a sim_size comparison we could use https://docs.galaxyproject.org/en/master/dev/schema.html#has-size with equivalent results

<option value="hide" selected="true">hide</option>
</param>
<when value="show">
<param name="rtrange" type="text" value="" label="RTrange" help="RT range to process in minutes, for example 5,25" >
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here an additional validator might be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

rt_range appears again below. Could be a macro

<option value="hide" selected="true">hide</option>
</param>
<when value="show">
<param name="db_input" type="data" format="msp,txt" label=" DB file" help="A database file as needed for DB option in runGC" />
Copy link
Contributor

Choose a reason for hiding this comment

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

What would be the txt input? Could be documented.

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.

9 participants