Validator::validateModel doesn't catch resets with variables outside the parent component

[kerimoyle]

Re: 12.1.1.1 "The value of the variable must be a variable reference to a variable defined within the component element parent of the reset element". Should the validateModel function catch this (added to TEST(Validator, resets) )

// adding reset to second component which does not contain the variable needed

	libcellml::ModelPtr m2 = std::make_shared<libcellml::Model>();
	libcellml::ComponentPtr c2 = std::make_shared<libcellml::Component>();
	libcellml::VariablePtr var2 = std::make_shared<libcellml::Variable>();
	libcellml::ResetPtr r8 = std::make_shared<libcellml::Reset>();
	libcellml::WhenPtr w3 = std::make_shared<libcellml::When>();

	w3->setOrder(20);
	w3->setCondition("<math xmlns=\"http:https://www.w3.org/1998/Math/MathML\"></math>");
	w3->setValue("<math xmlns=\"http:https://www.w3.org/1998/Math/MathML\"></math>");

	var2->setName("var2");
	var2->setUnits("metre");

	r8->setOrder(20);
	r8->addWhen(w3);
// 'var' is a valid variable defined earlier in this test, but is just not inside this component ...
	r8->setVariable(var); 
// the only variable inside this component is 'var2'
	c2->addVariable(var2);
	c2->addReset(r8); 
	c2->setName("comp2");
	m2->setName("main2");
	m2->addComponent(c2);

libcellml::Validator v2;
	v2.validateModel(m2);
	EXPECT_EQ(1u, v2.errorCount());

Currently the call to validate the m2 model returns no errors, but I would have expected something to come back? Have I missed something?

[hsorby]

A contextual note here would be that resets and their implementation are still under discussion so the reset validation hasn't been pushed very far.

Yes I realise that I haven't answered the question.

[hsorby]

To actually answer the question, yes it should raise an error.

[kerimoyle]

OK, thanks Hugh :) Might put this one further down the list until discussions are done

[hsorby]

Definitely a good choice there, I have pretty much left resets as is for a while.

[kerimoyle]

Just wanted to check in on this? How are the discussions about resets going ...?

[nickerso]

I believe the current plan is to get 0.9 out the door without any (minimal?) support for resets - that will still make sure that libCellML is able to support the vast majority of CellML 1.0/1.1 models that currently exist. And I think we really need that working implementation that we can use to test "experimental" support for resets to help us evaluate the various options for defining the resets in the specification.

[hsorby]

Yes let's release 0.9 as resets are (baring any major errors) and then look to generate models with reset and see what happens when we mix them with solvers.

[kerimoyle]

So just to tie up the original issue - should I add tests to the validator now which catch this error or leave it until after 0.9?

[MichaelClerx]

Syntactically I think resets are probably done! Will have this wrapped up in a few weeks hopefully....

[agarny]

So just to tie up the original issue - should I add tests to the validator now which catch this error or leave it until after 0.9?

For 0.9, I would have validation fail if a reset is found with a message about resets not being (currently) supported.

[kerimoyle]

Hi again all
I've added a few things to the validator around resets:

• First, any call to resets will generate a generic error that they're unsupported before version 1.0. It's not pretty, suggestions for better wording welcome!

// TODO Resets are not supported in version 0.9 so adding in an error here if they're present.
        ErrorPtr err = std::make_shared<Error>();
        err->setDescription("Component '"+component->getName()+"' contains reset items which are not supported prior to version 1.0.");
        err->setComponent(component);
        err->setRule(SpecificationRule::RESET_NOT_SUPPORTED);
        mValidator->addError(err);

• Secondly, resets are still checked after that ... (not sure if I should remove it all or not?) .. including that the variable is indeed present in the reset's parent component, which was the original issue.

Changes are in #304 now.

Two related things I noticed:

• In the error messages around "whens" some are capitalised to be Whens (sans "). Is there a better term we can call them which isn't so strange to read? And if Whens are capitalised, should Components, Variables etc be so too? Or none? Or called something else?
• Resets inherit the Entity->getParent() method, but it's never set to be the parent component. Is this intentional? Or just irrelevant because resets will never need to refer back to their own parent component?

[agarny]

For 0.9, I would remove anything that is related to resets in our validation code. Also, I wouldn't explicitly mention that resets "are not supported prior to version 1.0". I would simply say that they "are not currently supported". We clearly want to support them in version 1.0, but better cover our back... :)

• In the error messages around "whens" some are capitalised to be Whens (sans "). Is there a better term we can call them which isn't so strange to read? And if Whens are capitalised, should Components, Variables etc be so too? Or none? Or called something else?

Agreed, we should do something about our messages and make them consistent. Maybe this could be addressed as part of issue #314?

• Resets inherit the Entity->getParent() method, but it's never set to be the parent component. Is this intentional? Or just irrelevant because resets will never need to refer back to their own parent component?

Yes, that's a more general issue that I have discussed with @hsorby and @nickerso, and that is related to our current object model. I basically think we need to review it (e.g. a Units can currently have a component as a parent while this is not allowed in CellML 2.0).

[kerimoyle]

• I've removed all the reset validation from the PR Validation #304 now, but have kept the code and tests which deal with them in a branch here: https://github.com/kerimoyle/libcellml-keri/tree/reset_validation
• I haven't changed the parser to return errors when encountering whens or resets

[kerimoyle]

Added the validator and tests for this issue into PR #355