-
Notifications
You must be signed in to change notification settings - Fork 441
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
update proteinortho v6.2.3 and some changes to the xml #5184
update proteinortho v6.2.3 and some changes to the xml #5184
Conversation
Current problems:
|
The artifacts are here: https://github.com/galaxyproject/tools-iuc/actions/runs/4386446633 You can find them if you go to any of the tests and there go to the summary page. |
Thank you! |
<section name="more_options" title="Additional Options" expanded="False"> | ||
<param argument="--evalue" type="float" value="0.001" min="0" label="E-value threshold of the blast algorithm" help="Larger values results in more false positives (connections between proteins)."/> |
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.
For consistency I would use $more_options.evalue
in the command block (even if $evalue
should work as well).
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.
ahh ok, so since these options are nested in the more_options sections, the env var needs this too, I will add this to the xml
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 the test also need this? So e.g. <param name="evalue" value="1"/>
needs to be <param name="more_options.evalue" value="1"/>
?
I just saw that diamond wraps the nested params in the same section
tags, so this is probably the solution for this problem here
The html output is easier to read (if you click on failed / errored tests below the table the failing tests are shown). |
OK, i managed to pass all test when removing C2.faa (only for diamond). |
Cool. Looks good to me. For the problem with the citations I would suggest to check if they also appear if you start a local instance with I was also wondering if it would be better to include all the requirements in the bioconda recipe? But we don't have to do it now. Advantage would be that they would be included in the container that is built by bioconda.. And we could avoid building yet another container over here. |
FYI: the linter warns (for the summary tool) : |
The requirements are optional and not mandatory (only diamond+lapack are required). I can try to Thanks for mentioning the TODO in summary, I will work on resolving this! |
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 checked the citations with planemo serve
. They look good. So the problem seems to be at usegalaxy.eu. @bgruening can you check if something is wrong with the citations of proteinortho at the public server?
One last optional suggestion: you could set the column names metadata for the output tables:
<action name="column_names" type="metadata" default="Geneid,Chr,Start,End,Strand,Length,${alignment.element_identifier}"/> |
Leave this up to you. Just tell me if I should merge.
Update: I will try the I was not aware that you can put variables in the |
|
from_work_dir="result.proteinortho.tsv"> | ||
<actions> | ||
<action name="column_names" type="metadata" | ||
default="species,genes,alg.-conn.,${','.join([ f.element_identifier for f in $input_files ])}"/> |
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.
Lets see if this works. You should certainly check with planemo serve
if this works out as expected. Maybe also include it in the tests https://docs.galaxyproject.org/en/master/dev/schema.html#tool-tests-test-output-metadata
There are even more advanced techniques for setting output metadata. For instance you can deposit a file galaxy.json
in the working directory. That describes all sorts of output properties. Unfortunately this is largely undocumented https://planemo.readthedocs.io/en/latest/writing_advanced.html?highlight=galaxy.json#tool-provided-metadata
tools/proteinortho/proteinortho.xml
Outdated
@@ -4,7 +4,6 @@ | |||
<import>proteinortho_macros.xml</import> | |||
<xml name="test_outputs"> | |||
<output name="proteinortho"> | |||
<metadata name="column_names" value="species,genes,alg.-conn.,${','.join([ f.element_identifier for f in $input_files ])}"/> |
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 guess you could keep the metadata tag but you would need to replace ${','.join([ f.element_identifier for f in $input_files ])}
with a specific list of columns for the respective test. If this list is not constant we can parametrize the macro.
Let me know if you need help with this.
I think that is everything from my side! |
Hi @pmjklemm I have a few more commits over here https://github.com/bernt-matthias/tools-iuc/tree/tools/proteinortho for some reason I can not push to your branch or open a PR against your fork (probably because you forked from the autoupdate fork). If you like you can cherry pick them to here. Mostly stricter tests regarding column and row numbers (please take check test 3 .. row numbers seem to change from call to call .. so I added delta). Successful linting of the |
Yes, I tried to push to the autoupdate branch of the last PR of proteinortho and realized that this is not possible. Your additions look very good, I did take them all! And no hurry, we can wait on #15791 and merge afterwards. |
I will trigger a new test with v6.2.3 (just squashed in bioconda) |
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.
Still looks good. But we also still have the problem with the linter.
Btw. you can always trigger CI by closing and reopening a PR.
The linter problem should be fixed once galaxyproject/galaxy#15933 and galaxyproject/galaxy#15873 are merged and new python packages have been released. I'm not sure when this will happen .. people seem busy with the 23.0 release and courses. Tell me if its pressing. Then we can merge and deploy manually. |
It is not urgent, I can wait. From time to time I will check if it merges, so that is no problem (: |
Finally :) Thanks for all your efforts and patience. |
update to 6.2.0
FOR CONTRIBUTOR: