checkConsistencyWithStrictUnits reports Units issues as Errors not Warnings

[matthiaskoenig]

Hi all,
this is related to:
#378

I switched from checkConsistency to checkConsistencyWithStrictUnits and now get a lot of errors instead of warnings.
My understanding is that the function is just performing some additional more strict rules for unit validation to handle numerical conversion factors based on the documentation:

"Performs consistency checking and validation on this SBML document using the ultra strict units validator that assumes that there are no hidden numerical conversion factors."

But in addition the function reports units consistency issues as "error" not as "warning". This is incorrect. Despite the more strict checking the unit consistency issues are still warnings and the model is valid!
The unit consistency issues have to be reported as warnings, so that checkConsistency to checkConsistencyWithStrictUnits behave in the same way (with just additional tests in the strict version) and the validation result is in line with "units being annotations but not making the model invalid".

Best Matthias

[skeating]

Actually I understand where you're coming from with respect to the documentation you quote. But originally the ability to check units with full strictness was written so that any unit problem was flagged as an error and not a warning.

Can you give me an example of something that is reported as a strict error that you think should still only be a warning.

[matthiaskoenig]

I get this, but then the all the errors of the checkConsistencyWithStrictUnits should be warnings when using checkConsistency. I.e. the only difference between the two checks should be that the unit issues are reported either as warnings or errors, but it should be the same set of messages!.
I don't understand the additional difference of saying "there are some unit issues such as on conversion factors which we only report as errors when using the strict units".

[fbergmann]

We could change the API such that checkConsistencyWithStrictUnits gets an override flag, that will be passed on. That way callers can decide whether those unit issues are warnings / errors.

Since those unit tests are a bit expensive I don't think they ought to run by default with checkConsistency, so having a separate call with different messages is fine.

[matthiaskoenig]

@fbergmann That would be great. Otherwise I have to parse these myself and down-promote them to warnings on my side.

The point I don't understand is why there are differences in the tests between the two sets? The SBML specification states what are the errors/warnings/recommendations. These are defined in Appendix A "Validation and consistency rules for SBML".

When running checkConsistency I don't get the warning corresponding to the following error from checkConsistencyWithStrictUnits.

ERROR    E0: SBML unit consistency (core, L434, code)                                                                                                                                                                                               validation.py:189
         [Error] Mismatched units in assignment rule for parameter                                                                                                                                                                                                   
         When the 'variable' in an <assignmentRule> refers to a <parameter>, the units of the rule's right-hand side are expected to be consistent with the units declared for that parameter.                                                                       
         Reference: L3V2 Section 4.9.3                                                                                                                                                                                                                               
           Expected units are gram (exponent = 1, multiplier = 1, scale = 0) but the units returned by the <math> expression of the <assignmentRule> with variable 'UGE' are gram (exponent = 1, multiplier = 0.001, scale = 0).         

The checkConsistency must return all warnings that correspond to the checks for that document. Implementing a checkConsistency that only runs the tests that are performant does not make sense to me.
I immediately ask myself what other things are not being checked on the document and would never use the checkConsistency but always the strict version.

[fbergmann]

We always had options to turn on / off different validators. when I look at it it looks that checkConsistencyWithStrictUnits runs the same checks as checkConsistency, however it turns off the original unit checking validator from checkConsistency and instead runs a StrictUnitConsistencyValidator and for that one raises the error level from warning to error.

@skeating will know more about why that was done in this way.

[fbergmann]

Thinking about this a bit longer, I propose 2 changes:

• add a new LIBSBML_CAT_STRICT_UNITS_CONSISTENCY, that can be used with the normal internal validator by running SBMLDocument::setConsistencyChecks. Then we can have the behaviour that Matthias wants (the new error messages in checkConsistency)
• add a override flag to the strict check, so that callers can decide whether they want warnings or errors.

[luciansmith]

I assume the problems are flagged as errors because when the validator was written, they were errors. Then when later versions of the spec downgraded them, they were forgotten. At this point, they should definitely be warnings or errors based on the level/version of the document, just like the other unit validator.

As for why the two validators in the first place, it was originally impossible to flag a constant as having units, so the default validator had to assume that any stray constant was there to fix the units. A = B would be wrong when A and B had different units, but A = 1*B might be correct if '1' was in units of a conversion factor. This was common practice, and why the default validator assumed the user just got things right. But it wasn't guaranteed, so the more strict validator was written that did not make that assumption, to catch more errors.

[matthiaskoenig]

@luciansmith This makes a lot of sense. With the introduction of units via the `sbml:units' attribute in SBML Level 3, constants in mathematical expressions can be checked for units.

I.e. for SBML level 3 these should be warnings because either 1. there is no sbml:units, which is a units warning because the expression cannot be checked; or 2. there is an sbml:units and the expression can be checked (and a warning given if the units are wrong).

For SBML level 2, the strict and non-strict versions make sense, but not for SBML level 3.

I don't need the strict version because I don't work with SBML level 2 models at all, I just want the correct warnings on the SBML level 3 models. I hope that makes sense.