<degree> can be child of other stuff?

kerimoyle commented 4 years ago

See https://github.com/cellml/libcellml/pull/631/files#r448700802

agarny commented 4 years ago

Nice digging job! :)

nickerso commented 4 years ago

The spec is never wrong! In CellML, the degree element is only allowed as the child of root and diff expressions - there is no implication that the sentence means a child in terms of the XML serialisation.

agarny commented 4 years ago

Hmm... I still find it confusing. I would personally rephrase it to read "degree MUST be used in the context of a root or diff", or something like that, but spec writing clearly eludes me, so...

kerimoyle commented 4 years ago

If the spec is correct (however it's phrased) then the check has to happen in the Analyser/Validator instead?

agarny commented 4 years ago

The validator validates all the maths using the W3C MathML DTD, so we are fine.

kerimoyle commented 4 years ago

It doesn't check this CellML-specific restriction though ... which was the point of the discussion.

nickerso commented 4 years ago

Does it check that only the permitted MathML elements are allowed? In which case it would be checking this restriction by implication...

kerimoyle commented 4 years ago

It doesn't check where it happens, only that the tag is one of the CellML set (see below). The Analyser is prob the best place to check it as it's the only place where the mathml is actually interpreted into anything?

static const std::vector<std::string> supportedMathMLElements = {
    "ci", "cn", "sep", "apply", "piecewise", "piece", "otherwise", "eq", "neq", "gt", "lt", "geq", "leq", "and", "or",
    "xor", "not", "plus", "minus", "times", "divide", "power", "root", "abs", "exp", "ln", "log", "floor",
    "ceiling", "min", "max", "rem", "diff", "bvar", "logbase", "degree", "sin", "cos", "tan", "sec", "csc",
    "cot", "sinh", "cosh", "tanh", "sech", "csch", "coth", "arcsin", "arccos", "arctan", "arcsec", "arccsc",
    "arccot", "arcsinh", "arccosh", "arctanh", "arcsech", "arccsch", "arccoth", "pi", "exponentiale",
    "notanumber", "infinity", "true", "false"};

The DTD error is way too general for our specific case:

nickerso commented 4 years ago

if the DTD + restricted set of elements isn't enough, then I'd suggest this is something the validator should be checking in future as we look to make the AST more widely accessible. The analyser shouldn't be validating - that is why it will only work on valid models, right?

agarny commented 4 years ago

if the DTD + restricted set of elements isn't enough, then I'd suggest this is something the validator should be checking in future as we look to make the AST more widely accessible. The analyser shouldn't be validating - that is why it will only work on valid models, right?

I would indeed expect the Validator to do that kind of check, if deemed necessary.

kerimoyle commented 4 years ago

Closing in favour of libcellml-based discussion here

kerimoyle commented 4 years ago

The MathML2 manual says that this is valid:

<apply>
  <diff/>
  <bvar>
    <ci> x </ci>
    <degree><cn> 2 </cn></degree>
  </bvar>
  <apply><fn><ci>f</ci></fn>
    <ci> x </ci>
  </apply>
</apply>

In other words, <degree> is a direct child of <bvar> rather than <diff>. Is this ok in CellML? I'm struggling to see how it's allowed as a child of diff and not a child of bvar too ... ?

agarny commented 4 years ago

As far as I know/remember, we don't support the fn tag. As for

In other words, <degree> is a direct child of <bvar> rather than <diff>. Is this ok in CellML? I'm struggling to see how it's allowed as a child of diff and not a child of bvar too ... ? Yes, degree must be a direct child of bvar. When it comes to the CellML specification, I feel like it's ambiguous ("degree MUST be a child of root or diff"), but I understand that @nickerso thinks it's fine.

Anyway, we do have a test for this MathML code (in tests/resources/generator/non_first_order_odes.cellml):

<math xmlns="http://www.w3.org/1998/Math/MathML">
    <apply>
        <eq/>
        <apply>
            <diff/>
            <bvar>
                <ci>time</ci>
                <degree>
                    <cn cellml:units="dimensionless">2</cn>
                </degree>
            </bvar>
            <ci>z</ci>
        </apply>
        <cn cellml:units="per_second">1</cn>
    </apply>
</math>

We also have some MathML code to test degree in a root (in tests/resources/generator/coverage/model.cellml):

<math xmlns="http://www.w3.org/1998/Math/MathML">
    <apply>
        <eq/>
        <ci>eqnRootSqrtOther</ci>
        <apply>
            <root/>
            <degree>
                <cn cellml:units="dimensionless">2</cn>
            </degree>
            <ci>m</ci>
        </apply>
    </apply>
    <apply>
        <eq/>
        <ci>eqnRootCube</ci>
        <apply>
            <root/>
            <degree>
                <cn cellml:units="dimensionless">3</cn>
            </degree>
            <ci>m</ci>
        </apply>
    </apply>
    <apply>
        <eq/>
        <ci>eqnRootCi</ci>
        <apply>
            <root/>
            <degree>
                <ci>n</ci>
            </degree>
            <ci>m</ci>
        </apply>
    </apply>
</math>

kerimoyle commented 4 years ago

Those are in the generator, not the validator ...

agarny commented 4 years ago

I know, I was merely pointing out how degree is to be used in MathML since you were wondering about its validity in CellML (for the bvar vs. diff bit).

kerimoyle commented 4 years ago

Yes, so the spec should be updated to make it clear that children of bvar are allowed too?

agarny commented 4 years ago

It's not that they are allowed too, but rather that they should be a child of a bvar, not a diff (if I recall correctly). When it comes to the spec, and as I said, I understand that @nickerso is fine with it as it is...

kerimoyle commented 4 years ago

After discussion this evening (@hsorby @agarny @kerimoyle) we found that:

degree can never be a child of <root/> as it never has children.
degree can never be a child of <diff/> as it doesn't have children either. ... therefore, the spec line below is poorly worded and needs to change:

But, we didn't know what the purpose of the asterisk in the spec here actually is? The only operators allowed in CellML which permit the <degree> qualifier are the <root> and <diff> anyway, so it seems strange to spell it out explicitly, especially as we don't say anywhere that the MathML itself has to be valid. Thoughts, @nickerso @MichaelClerx ? I vote we delete the asterisk!

MichaelClerx commented 4 years ago

I think it doesn't go in a diff but in a bvar?

But otherwise agreed: Only elements where it can validly appear are bvar and root so no need to spell that out.

cellml / cellml-specification

<degree> can be child of other stuff? #335