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

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

Open
kerimoyle opened this issue Aug 6, 2020 · 19 comments
Open

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

kerimoyle opened this issue Aug 6, 2020 · 19 comments

Comments

@kerimoyle
Copy link
Collaborator

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

@agarny
Copy link
Contributor

agarny commented Aug 6, 2020

Nice digging job! :)

@nickerso
Copy link
Contributor

nickerso commented Aug 6, 2020

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
Copy link
Contributor

agarny commented Aug 6, 2020

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
Copy link
Collaborator Author

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

@agarny
Copy link
Contributor

agarny commented Aug 6, 2020

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

@kerimoyle
Copy link
Collaborator Author

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

@nickerso
Copy link
Contributor

nickerso commented Aug 6, 2020

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

@kerimoyle
Copy link
Collaborator Author

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:

W3C MathML DTD error: Element apply content does not follow the DTD, expecting (csymbol | ci | cn | apply | reln | lambda | condition | declare | sep | semantics | annotation | annotation-xml | integers | reals | rationals | naturalnumbers | complexes | primes | exponentiale | imaginaryi | notanumber | true | false | emptyset | pi | eulergamma | infinity | interval | list | matrix | matrixrow | set | vector | piecewise | lowlimit | uplimit | bvar | degree | logbase | momentabout | domainofapplication | inverse | ident | domain | codomain | image | abs | conjugate | exp | factorial | arg | real | imaginary | floor | ceiling | not | ln | sin | cos | tan | sec | csc | cot | sinh | cosh | tanh | sech | csch | coth | arcsin | arccos | arctan | arccosh | arccot | arccoth | arccsc | arccsch | arcsec | arcsech | arcsinh | arctanh | determinant | transpose | card | quotient | divide | power | rem | implies | vectorproduct | scalarproduct | outerproduct | setdiff | fn | compose | plus | times | max | min | gcd | lcm | and | or | xor | union | intersect | cartesianproduct | mean | sdev | variance | median | mode | selector | root | minus | log | int | diff | partialdiff | divergence | grad | curl | laplacian | sum | product | limit | moment | exists | forall | neq | factorof | in | notin | notsubset | notprsubset | tendsto | eq | leq | lt | geq | gt | equivalent | approx | subset | prsubset | mi | mn | mo | mtext | ms | mspace | mrow | mfrac | msqrt | mroot | menclose | mstyle | merror | mpadded | mphantom | mfenced | msub | msup | msubsup | munder | mover | munderover | mmultiscripts | mtable | mtr | mlabeledtr | mtd | maligngroup | malignmark | maction)*, got (CDATA bvar ).

@nickerso
Copy link
Contributor

nickerso commented Aug 6, 2020

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
Copy link
Contributor

agarny commented Aug 6, 2020

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
Copy link
Collaborator Author

Closing in favour of libcellml-based discussion here

@kerimoyle
Copy link
Collaborator Author

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 ... ?

@kerimoyle kerimoyle reopened this Aug 9, 2020
@agarny
Copy link
Contributor

agarny commented Aug 9, 2020

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
Copy link
Collaborator Author

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

@agarny
Copy link
Contributor

agarny commented Aug 9, 2020

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
Copy link
Collaborator Author

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

@agarny
Copy link
Contributor

agarny commented Aug 9, 2020

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
Copy link
Collaborator Author

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:

image

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
Copy link
Contributor

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.

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

No branches or pull requests

4 participants