search

biosimulators / Biosimulators_utils

Utilities for building standardized command-line interfaces for biosimulation software packages
https://docs.biosimulators.org/Biosimulators_utils
MIT License
4 stars 6 forks source link

feat: check all SED-ML XPath targets and warn appropriately #102

Closed luciansmith closed 2 years ago

luciansmith commented 2 years ago

This was part of the earlier change but somehow got dropped.

EDIT: fundamental changes due to comments; see below.

jonrkarr commented 2 years ago

I removed this because such attribute changes can also make changes that can break other XPaths. While this might not be the typical use pattern, this is possible. More nuanced analysis is needed to evaluate this.

jonrkarr commented 2 years ago

Basically, every change should be considered a "structural" change. This is one of the insidious features of XPaths. For many reasons, I'd advocate L2 SED-ML use something else.

luciansmith commented 2 years ago

OK, in that case I think we should validate the XPaths but put in warnings instead. Not validating the XPaths at all seems like a waste. (And again, I'm getting pages and pages of 'didn't check this xpath; didn't check the other xpath', which is not very helpful.)

jonrkarr commented 2 years ago

The XPaths are still validated and a warning is generated

jonrkarr commented 2 years ago

Could you add unit tests which illustrate the cases this impacts?

jonrkarr commented 2 years ago

@luciansmith it looks like you're still working on this. I converted this to draft so I know not to merge this yet. When its ready, click the "Ready for review" button below.

luciansmith commented 2 years ago

OK! Major update that fundamentally changes the goal of this pull request. See https://github.com/biosimulators/Biosimulators_utils/pull/102/commits/e72edd4bf667dc3a0ac03586accc384da195da90 for details, but in general: given that attribute changes can make XPaths valid or invalid, we just acknowledge this everywhere, and validate targets anyway.

luciansmith commented 2 years ago

Sure, adding tests is a good idea.

jonrkarr commented 2 years ago

Because this can lead to the false identification of errors, such issues should be reported as warnings. An additional option (whose default value is OFF) could be added to report them as errors.

luciansmith commented 2 years ago

Correct! XPath validation errors are only treated as errors if there are no model changes. See https://github.com/biosimulators/Biosimulators_utils/blob/check-datagens/biosimulators_utils/sedml/validation.py#L1659

jonrkarr commented 2 years ago

That works. This should help close the remaining gap in the validation of the SED-ML files from BioModels.

luciansmith commented 2 years ago

I'm having a little trouble figuring out how best to fit the design of your unit test system. I have some SED-ML files that should each produce a different suite of errors and warnings, but I'm not sure how best to wrap them into your system. Should the SED-ML of each be created from scratch? Should I load just one and make changes to create the others internally? Should I just save all four and load them?

four_sedml_files.zip

The results should be:

No change, good xpath: no errors, no warnings No change, bad xpath: error for bad xpath, no warnings Change, good xpath: no errors, one general 'xpaths might be bad' warning. Change, bad xpath: no errors, one 'xpaths might be bad' warning, and one 'the xpath is bad but might be fixed by change' warning.

I also feel like xpaths for elements other than data generators should be tested in the presence/absence of a model change, but figured the datagens could be first.

jonrkarr commented 2 years ago

The SED-ML can either be generated in the code, or it could saved to the repository in tests/fixtures.

jonrkarr commented 2 years ago

The new test cases can be added to tests/sedml/test_sedml_validation.py, or you could start a new file in the same directory.

luciansmith commented 2 years ago

OK! I'll do that, then.

luciansmith commented 2 years ago

This should be good to go!

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 8 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

codecov[bot] commented 2 years ago

Codecov Report

Merging #102 (6d0bd63) into dev (9dde3e9) will decrease coverage by 0.01%. The diff coverage is 95.45%.

@@            Coverage Diff             @@
##              dev     #102      +/-   ##
==========================================
- Coverage   96.68%   96.66%   -0.02%     
==========================================
  Files          87       87              
  Lines        9294     9309      +15     
==========================================
+ Hits         8986     8999      +13     
- Misses        308      310       +2
Flag Coverage Δ
unittests 96.66% <95.45%> (-0.02%) :arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
biosimulators_utils/sedml/validation.py 99.57% <95.23%> (-0.21%) :arrow_down:
biosimulators_utils/_version.py 100.00% <100.00%> (ø)
biosimulators_utils/sedml/data_model.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4afc7e8...6d0bd63. Read the comment docs.

jonrkarr commented 2 years ago

Will be released as 0.1.170 in a few minutes.