Skip to content

Real numbers: tests for a leading plus sign, degenerate strings and empty cn elements - #1516

Open
LunarCapsule127 wants to merge 1 commit into
cellml:mainfrom
LunarCapsule127:issue1505
Open

LunarCapsule127 wants to merge 1 commit into
cellml:mainfrom
LunarCapsule127:issue1505

Conversation

@LunarCapsule127

Copy link
Copy Markdown

Addresses issue #1505.

As agreed on the issue, this PR adds tests only. It doesn't change the library, so we can agree on the expected behavior before I work on the fix. Most of the new tests fail on purpose for now. The branch is based on main, since there's no develop branch any more (docs/contributing.rst still mentions develop).

What the tests cover

Real numbers in unit exponent and multiplier attributes, variable initial values, MathML cn elements, and cn elements inside degree, bvar and logbase:

  • A leading + is accepted (section 1.3.3.2), including in e-notation.
  • The degenerate strings -., - and . are rejected.
  • Empty cn elements are rejected.

Every expected message reuses an existing parser or validator message, so there's no new wording to review.

Current results

Test Result now
Parser.unitAttributesWithLeadingPlusSign Fails: 4 "not a representation of a CellML real valued number" issues
Parser.unitAttributesWithDegenerateRealNumbers Fails: uncaught std::invalid_argument
Validator.variableInitialValuesWithLeadingPlusSign Fails: 3 invalid initial value issues
Validator.variableInitialValuesWithDegenerateRealNumbers Fails: no issues reported
Validator.mathCnElementsWithLeadingPlusSign Fails: 3 cn format issues
Validator.mathCnElementsWithDegenerateRealNumbers Fails: uncaught std::invalid_argument
Validator.mathEmptyCnElements Passes (already handled), kept as a guard
Validator.mathDegreeElementsWithLeadingPlusSign Passes, but only because these values aren't checked yet
Validator.mathDegreeElementsWithDegenerateRealNumbers Fails: no issues reported
Validator.mathDegreeElementsWithEmptyCnElements Fails: no issues reported
Validator.mathLogbaseElementsWithDegenerateRealNumbers Fails: no issues reported
Validator.mathLogbaseElementsWithEmptyCnElements Fails: no issues reported

All existing tests still pass.

What I found while writing them

  • There are two causes, not one. isCellMLBasicReal() in src/utilities.cpp removes the sign and the decimal point, then runs std::all_of on what's left. For -., - and . nothing is left, and std::all_of on an empty range returns true. It also doesn't allow a leading +. Separately, validateMathMLElementsChildrenAndSiblings() only looks inside apply, piecewise, piece and otherwise, so a cn inside degree, bvar or logbase is never checked.
  • Degenerate strings throw instead of being reported. In unit attributes and cn elements, - and . reach std::stod. stringToDouble() only catches std::out_of_range, so std::invalid_argument escapes from parseModel() and validateModel(). Initial values only go through isCellMLReal(), so there they are accepted without any issue.
  • The specification's examples disagree with its rule. Rule 1.3.3.2 says a basic real number string MAY begin with +, but the examples in the same section list "+123.4567" and "+1.234E03" as not valid. I've followed the rule, as suggested on the issue, but the examples may need a fix in the specification.

Left out on purpose

As suggested on the issue, there are no tests for out-of-range numbers such as 1e999. For the separate analyser issue, this is how they behave now:

  • Unit exponent and multiplier: the parser reports that the value is out of range of the double type (1e-999 too).
  • Variable initial values: accepted, and the generator then emits 1.0e999.
  • Real cn: rejected, but only because a real cn can't contain e.
  • e-notation cn such as 1<sep/>999: accepted.

Questions

  1. Should the degree, bvar and logbase fix go in this issue, or in a separate one?
  2. Should a leading + be accepted in e-notation cn and in degree in the same way? The tests currently assume yes.
  3. Should unit attributes keep rejecting out-of-range values in the parser? Today they behave differently from initial values. I'm happy to leave this for the analyser issue too.

@hsorby Thank you for the welcome! No rush at all on the review. ✨

Addresses issue cellml#1505.

Add parser and validator tests describing the expected behavior for
real numbers entered through unit exponent and multiplier attributes,
variable initial values, MathML cn elements, and cn elements inside
degree and logbase elements:

- a leading '+' sign is valid (CellML 2.0 section 1.3.3.2);
- the degenerate strings '-.', '-' and '.' are invalid;
- empty cn elements are invalid.

These tests are expected to fail until the implementation is updated.
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.

1 participant