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

influx_si v7.0.1_1 #255

Merged
merged 10 commits into from
Dec 13, 2023
Merged

influx_si v7.0.1_1 #255

merged 10 commits into from
Dec 13, 2023

Conversation

sgsokol
Copy link
Contributor

@sgsokol sgsokol commented Nov 23, 2023

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)

Signed-off-by: Serguei Sokol <[email protected]>
Signed-off-by: Serguei Sokol <[email protected]>
Signed-off-by: Serguei Sokol <[email protected]>
@sgsokol sgsokol changed the title v7.0.1_1 influx_si v7.0.1_1 Nov 24, 2023
@sgsokol
Copy link
Contributor Author

sgsokol commented Nov 28, 2023

Please review and deploy if OK.

tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/environment.yml Outdated Show resolved Hide resolved
tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
tools/influx_si/README.rst Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/macros.xml Outdated Show resolved Hide resolved
tools/influx_si/environment.yml Outdated Show resolved Hide resolved
tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
tools/influx_si/influx_si.xml Show resolved Hide resolved
tools/influx_si/influx_si.xml Show resolved Hide resolved
tools/influx_si/influx_si.xml Outdated Show resolved Hide resolved
Signed-off-by: Serguei Sokol <[email protected]>
Signed-off-by: Serguei Sokol <[email protected]>
@sgsokol
Copy link
Contributor Author

sgsokol commented Dec 8, 2023

FOR CONTRIBUTOR:

* [ ]  - I have read the [CONTRIBUTING.md](https://github.com/galaxyproject/tools-metabolomics/blob/master/CONTRIBUTING.md) document and this tool is appropriate for the tools-iuc repo.

The URL for CONTRIBUTING.md is broken. Probaly it should be https://github.com/workflow4metabolomics/tools-metabolomics/blob/master/CONTRIBUTING.md

@sgsokol
Copy link
Contributor Author

sgsokol commented Dec 8, 2023

All issues seem to be resolved to me. Do you see any left out?

@MathsCell
Copy link

Please merge if no further action is required on my part.

@lecorguille
Copy link
Member

Thank you @bgruening and @bernt-matthias for your reviews! You rocks!

Thank you @sgsokol for your contribution and to have apply the advices and requests.

$opt.fullsys
$opt.emu
$opt.irand
<tool id="influx_si" name="influx_si" version="@TOOL_VERSION@+galaxy1" profile="21.09">
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="influx_si" name="influx_si" version="@TOOL_VERSION@+galaxy1" profile="21.09">
<tool id="influx_si" name="influx_si" version="@TOOL_VERSION@+galaxy0" profile="21.09">

<param argument="--iseries" type="text" value="" label="--iseries" optional="true" help="Indexes of starting points to use. Format: &#x27;1:10&#x27; -- use only first ten starting points; &#x27;1,3&#x27; -- use the the first and third starting points; &#x27;1:10,15,91:100&#x27; -- a mix of both formats is allowed. Default: &#x27;&#x27; (empty, i.e. all provided starting points are used)" />
<param argument="--seed" type="integer" min="0" value="" label="--seed" optional="true" help="Integer (preferably a prime integer) used for reproducible random number generating. It makes reproducible random starting points (--irand) but also Monte-Carlo simulations for sensitivity analysis. Default: none, i.e. current system value is used, so random drawing will be varying at each run." />
<param argument="--excl_outliers" type="float" min="0" max="1" value="" label="--excl_outliers" optional="true" help="This option takes an optional argument, a p-value between 0 and 1 which is used to filter out measurement outliers. The filtering is based on Z statistics calculated on reduced residual distribution. Default: 0.01." />
<param argument="--tblimit" type="integer" min="0" value="0" label="--tblimit" optional="true" help="developer option: set trace back limit for python error messages" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Labels should be reverted, like so:

Suggested change
<param argument="--tblimit" type="integer" min="0" value="0" label="--tblimit" optional="true" help="developer option: set trace back limit for python error messages" />
<param argument="--tblimit" type="integer" min="0" value="0" label="Python traceback limit" optional="true" help="developer option: set trace back limit for python error messages" />

for all parameters. Or if you added new parameters the label should be a short description of the parameter. (The argument is already automatically added to the help. So its clear which Galaxy parameter corresponds to which command line argument.)

<when value="s"/>
<when value="i">
<param argument="--time_order" type="select" display="radio" label="Time order for ODE solving" help="Order 2 is more precise but more time consuming than order 1. The value &#x27;1,2&#x27; makes to start solving the ODE with the first order scheme then continues with the order 2.">
<option value="None">From .opt file or Default</option>
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to clarify what the .opt file is and how to specify it.

</section>
</inputs>
<outputs>
<collection name="influx_si_output" type="list:list" label="influx_${si.s_i}_on_${on_string}">
Copy link
Contributor

Choose a reason for hiding this comment

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

I would still suggest to create multiple output collections. One for each output type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. Do you have examples to point me to?

Copy link
Contributor

Choose a reason for hiding this comment

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

The planemo doc cover this a bit. In general the outputs section can contain an arbitrary number of collection and dataset outputs.

Something like this:

<collection name="influx_si_output" type="list:list" label="influx_${si.s_i}_on_${on_string}: log and error files">
    <discover_datasets match_relative_path="true" recurse="true" pattern="(?P&lt;identifier_0&gt;[^/]+)_res/(?P&lt;identifier_1&gt;[^/]+)\.err" ext="txt"/>
    <discover_datasets match_relative_path="true" recurse="true" pattern="(?P&lt;identifier_0&gt;[^/]+)_res/(?P&lt;identifier_1&gt;[^/]+)\.log" ext="txt"/>
</collection>
<collection name="influx_si_output" type="list:list" label="influx_${si.s_i}_on_${on_string}: sim and stats files">
    <discover_datasets match_relative_path="true" recurse="true" pattern="(?P&lt;identifier_0&gt;[^/]+)_res/(?P&lt;identifier_1&gt;[^/]+)\.sim" ext="txt"/>
    <discover_datasets match_relative_path="true" recurse="true" pattern="(?P&lt;identifier_0&gt;[^/]+)_res/(?P&lt;identifier_1&gt;[^/]+)\.stats" ext="txt"/>
</collection>

The first collection contains the log and error files and the second the sim and stats files. Bu using the ext attribute in discover_datasets we set the extension and do not need to move.

If you like to have the extensions in the indentifiers then you could use a discovery rule like (i.e. make the file extension part of the identifier_1 pattern):

    <discover_datasets match_relative_path="true" recurse="true" pattern="(?P&lt;identifier_0&gt;[^/]+)_res/(?P&lt;identifier_1&gt;[^/]+\.stats)" ext="tsv"/>

For really large numbers of possible outputs filters are also a nice option.

Comment on lines 64 to 66
#if $opt.np:
--np='$opt.np'
--np='$opt.np'
#end if
Copy link
Contributor

Choose a reason for hiding this comment

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

The number of CPU cores must not be under the users control. Use

Suggested change
#if $opt.np:
--np='$opt.np'
--np='$opt.np'
#end if
--np='\${GALAXY_SLOTS:-1}'

Then Galaxy will set it automatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And what if user wants to run it sequentially? Or does not want to occupy all cores but only half of them? Or only 2 (as more cores does not necessarily accelerate the calculation but only wastes the ressources)?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a thing a user should not be bothered with. For Galaxy the user does not even know where the code runs. Some Galaxy servers user HPC systems scattered over different countries.

Bottom line is that this should be configured by the admin/galaxy.

There is something called ressource parameters that admins can enable for some tools/users .. then the number of cores would be controllable by the user.

@lecorguille lecorguille merged commit 77b389c into workflow4metabolomics:master Dec 13, 2023
9 checks passed
@sgsokol sgsokol deleted the main branch December 13, 2023 09:39
@sgsokol
Copy link
Contributor Author

sgsokol commented Dec 15, 2023

Thank you @bgruening and @bernt-matthias for your reviews! You rocks!

Thank you @sgsokol for your contribution and to have apply the advices and requests.

Thanks to all for your advices and corrections.

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.

5 participants