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

mz(X)MLMetadataCollector #280

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

Conversation

Hrodwin
Copy link

@Hrodwin Hrodwin commented Dec 22, 2024

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)

Hi, beginning the request for the deployment of a new tool gathering metadata from mzml and mzxml files: mz(X)MLMetadataCollector.

@Hrodwin Hrodwin changed the title Adding tool mz(X)MLMetadataCollector Dec 22, 2024
@Hrodwin
Copy link
Author

Hrodwin commented Dec 23, 2024

Hi, many adjustments but tool finally ready for a last checkup by the team before merging! Unfortunatelly test files have already been reduced to minimum and could not meet the size criteria..

tools/mzxmlmetadatacollector/mzXMLMetadataCollector.xml Outdated Show resolved Hide resolved
<help><![CDATA[
.. class:: infomark

**Credits**
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the creators tag to provide credits: https://docs.galaxyproject.org/en/latest/dev/schema.html#tool-creator-person

Copy link
Member

Choose a reason for hiding this comment

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

I am unsure how to specify the roles mentionned here using the creator tag. Would it be ok to simply add the creator tag as a complement for information about Quentin as the tool developer?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just adding recommendations, you are free to ignore it :)

Do you miss anything in the creator tags that we should add? Those tags have been added to acknowledge tool developers and contributors.

Copy link
Member

Choose a reason for hiding this comment

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

I have made a suggestion to add the tag in the XML (see dedicated comment).

The additional information I am unsure where to put using the creator tag is the role. I see there is a jobTitle tag, but I do not know whether it can be used to put this kind of information? As it is described in schema.org, it seems to be more about one's overall position, not really a specific role at a given scale. But I may very probably be wrong here as I am unfamiliar with these tags, actually. What I would have liked would have been to have an entry where I can put things like "original code developper" , "previous versions' participation", "maintainer", "interface supervision" for example. But again it may not be relevant, I can not gauge the usefullness in regard to the effort it would be to have an additional entry in this creator tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a general comment, I think this script would be much more robust if it used a XML parser like lxml

Copy link
Member

Choose a reason for hiding this comment

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

I am afraid this is currently out of my scope of expertise to evaluate that (I have basic skills in Python but have no regular programming tasks to be fluent in this language). Since Quentin's temporary contract has come to term, he has no longer dedicated time to work on this. So I guess it will stay as it is for now, if it is not a limiting issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess your decision. It would happen that if you get slightly dirty XML with whitespace and such, that your hand-crafted parser will not work etc ... But that a theoretical problem, no clue if this is a practical concern.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the question of robustness to XML files' variations has been discussed previously with the users asking the tool to be shared, before we decide to wrap it. And they do acknowledge that in case the XML generation they performe may differ from the various-but-still-not-exhaustive use-cases with which the original tool has been used, it may lead to impossibility to correctly retreive the wanted information. Thus we added, when wrapping the tool, a comment in a "known issue" section of the tool help, for people to be aware of the limitation.

@melpetera
Copy link
Member

Hi @bgruening and thank you for the review.

As @Hrodwin was in temporary contract position in my lab and the contract has come to term, I am in charge of finalizing his Galaxy tools' submission. So I have tried answering the comments, for as far as I can. In case there are limiting points I can not address relevantly I will contact him, but since he no longer has dedicated time to work on it I will try to handle all I can.

</help>
<citations>
<citation type="doi">10.1093/bioinformatics/btu813</citation>
</citations>
Copy link
Member

@melpetera melpetera Jan 21, 2025

Choose a reason for hiding this comment

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

Would this be ok as creator tags?

Suggested change
</citations>
</citations>
<creator>
<person givenName="Quentin" familyName="Ruin" jobTitle="Research engineer"></person>
<organization name="PFEM" url="https://eng-pfem.isc.inrae.fr/"></organization>
<organization name="MetaboHUB" url="https://www.metabohub.fr/home.html"></organization>
</creator>

</description>
<requirements>
<requirement type="package" version="2.1.0">numpy</requirement>
</requirements>
Copy link
Member

Choose a reason for hiding this comment

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

Following the pulsarification things, should we add this in the tool?

Suggested change
</requirements>
</requirements>
<required_files>
<include path="mzXMLMetadataCollector.py" />
</required_files>

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.

3 participants