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

MzTab Validation #362

Merged
merged 42 commits into from
Aug 7, 2024
Merged

MzTab Validation #362

merged 42 commits into from
Aug 7, 2024

Conversation

Lilferrit
Copy link
Contributor

Automatically runs mzTab validation via github actions using a compiled jmzTab validator executable.

@Lilferrit
Copy link
Contributor Author

Lilferrit commented Aug 2, 2024

I am not looking for a review just yet I just opened this PR to run the test jobs and test the jmzTab validator steps.

@bittremieux bittremieux marked this pull request as draft August 3, 2024 07:40
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.03%. Comparing base (9e3b630) to head (e6f9185).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #362      +/-   ##
==========================================
+ Coverage   91.48%   94.03%   +2.54%     
==========================================
  Files          12       12              
  Lines        1022     1022              
==========================================
+ Hits          935      961      +26     
+ Misses         87       61      -26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Lilferrit Lilferrit marked this pull request as ready for review August 6, 2024 00:24
@Lilferrit Lilferrit requested a review from bittremieux August 6, 2024 00:24
tests/mzTab_validation/ting_config.yaml Outdated Show resolved Hide resolved
tests/mzTab_validation/validateMzTab.bat Outdated Show resolved Hide resolved
tests/mzTab_validation/validateMzTab.sh Outdated Show resolved Hide resolved
tests/mzTab_validation/tiny_model.ckpt Outdated Show resolved Hide resolved
@Lilferrit Lilferrit requested a review from bittremieux August 6, 2024 21:25
@Lilferrit Lilferrit linked an issue Aug 6, 2024 that may be closed by this pull request
Copy link
Collaborator

@bittremieux bittremieux left a comment

Choose a reason for hiding this comment

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

Nice clean implementation. 👍 One minor style comment.

One more thing that you might investigate a bit more before merging though is whether the validation can still be extracted. I'm thinking something like a separate GitHub action, where it now says "Set up Java" and uses that specific action, that we might create a dedicated action that provides mzTab validation. I haven't done something like that myself yet, so I don't know how it would work or whether it'd be possible.

The immediate benefit is that the jar would not need to be included in the Casanovo repository, but rather in a separate repository that would implement this action. The bigger benefit, however, would be that we'd then be able to easily call this action for any tool that exports to mzTab, whereas now the validation is Casanovo-specific.

tests/test_integration.py Show resolved Hide resolved
@bittremieux
Copy link
Collaborator

Hmm, question though: the change to export the precursor charge as an integer instead of a float in 9792584 doesn't seem to be retained in the final diff. Did that get reverted? And if so, the generated mzTabs should be non-compliant, so the test should fail. Can you double-check that the output is correct and the tests run successfully?

The precursor charge export fix should also be mentioned in the changelog.

@Lilferrit
Copy link
Contributor Author

Lilferrit commented Aug 7, 2024

Hmm, question though: the change to export the precursor charge as an integer instead of a float in 9792584 doesn't seem to be retained in the final diff. Did that get reverted? And if so, the generated mzTabs should be non-compliant, so the test should fail. Can you double-check that the output is correct and the tests run successfully?

The precursor charge export fix should also be mentioned in the changelog.

I checked ms_io.py on the latest commit on this branch and the precursor charge fix is still there. At one point I removed the fix to make sure that the pipeline fails if the output file isn't compliant, but I simply put the fix back when I was done. This probably messed with the git diff. The changelog still has the fix mentioned in it as well.

@bittremieux
Copy link
Collaborator

Yes, you're right. Very weird, the diff doesn't even indicate that ms_io.py was changed, nor the changelog.

@Lilferrit Lilferrit merged commit a46f995 into dev Aug 7, 2024
7 checks passed
@Lilferrit Lilferrit deleted the 322-mztab-validation branch August 7, 2024 21:03
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.

Automate mzTab validation
2 participants