17.1.2 Connections with self permitted in parser and validator

[kerimoyle]

See 17.1.2: Just wanted to check that the situation of a connection having identical component_1=component_2 is not permitted? Currently this is getting through the parser and the validator:

Parser:

TEST(Parser, connectionComponent1EqualComponent2) {
    /// @cellml2_17 17.1.2 Parser TEST Check component1 is not equal to component2 in a connection element
    const std::string e =
        "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n"
        "<model xmlns=\"http:https://www.cellml.org/cellml/2.0#\">"
        "<component name=\"robert\">"
        "<variable name=\"bob\" units=\"dimensionless\"/>"
        "</component>"
        "<connection component_1=\"robert\" component_2=\"robert\">"
        "<map_variables variable_2=\"bob\" variable_1=\"bob\"/>"
        "</connection>"
        "</model>";
    libcellml::Parser parser;
    parser.parseModel(e);
    for (size_t i = 0; i < parser.errorCount(); ++i) { // ***** error count is 0 ******
        std::cout << parser.getError(i)->getDescription() << std::endl;
    }
}

Validator:

TEST(Validator, validateConnectionComponent1NotEqualComponent2) {

    libcellml::Validator v;
    libcellml::ModelPtr m = std::make_shared<libcellml::Model>();
    libcellml::ComponentPtr comp7 = std::make_shared<libcellml::Component>();
    libcellml::VariablePtr v7 = std::make_shared<libcellml::Variable>();
    m->setName("modelName");
    v7->setName("variable7");
    v7->setUnits("dimensionless");
    comp7->setName("component7");
    comp7->addVariable(v7);
    m->addComponent(comp7);
    libcellml::Variable::addEquivalence(v7, v7);          
    v.validateModel(m);

    for (size_t i = 0; i < v.errorCount(); ++i) {
        std::cout << v.getError(i)->getDescription() << std::endl;
    }
}

... because of this bit of code. First time around it's set, second time does not enter the if statement.

void Variable::VariableImpl::setEquivalentTo(const VariablePtr &equivalentVariable)
{
	/// @cellml2_17 17.1.3 Note: variable equivalence pairs are not duplicated as existing v1-v2 connections are skipped
    /// @cellml2_19 19.10.4 Note: variable equivalence pairs are not duplicated as existing v1-v2 connections are skipped
    if (!hasEquivalentVariable(equivalentVariable)) {
        VariableWeakPtr weakEquivalentVariable = equivalentVariable;
        mEquivalentVariables.push_back(weakEquivalentVariable);
    }
}

Thoughts?

[agarny]

I would say that the parser should allow this (since it respects CellML's syntax), but the validator should definitely report connections with self (since it doesn't respect CellML's semantics).

[hsorby]

This part of the code is not the problem with the validation. The job of this part of the code is to set variable equivalence and that's it. The section of code that deals with validation is in the src/validator.cpp file.

The parser should definitely allow this to parse.

[kerimoyle]

@hsorby I'd thought that the point of the if (!hasEquivalentVariable(equivalentVariable)) statement in that function was to prevent duplicate equivalences from occurring ... but we can't use the same function to also prevent self-referencing? Can you help me understand the philosophy for that ... ?

I've added code to the validator to return an error if self-equivalence is found:
"Variable 'doppelganger' has an equivalent variable 'doppelganger' equal to itself. "

[hsorby]

Yes it is to prevent duplicate equivalences from occurring but it is not for preventing self equivalences.

[kerimoyle]

... but is there a reason it can't do both? I mean, if the code is already preventing one error (instead of letting the validator do it) is there a design rationale that says it can't prevent others too?

[hsorby]

There is a design rationale it's to stop you adding the same equivalence over and over again once you have added it there is no need to add any more. A smaller stack of equivalent variables is easier to manage and iterate over. The intent of the function is to set one variable equivalent to another not to prevent self equivalence.

I think it is also important to note that we are not trying to stop users from creating self-equivalences. The less we dictate how the library is used the better in my opinion.

[kerimoyle]

Self-equivalence is now tested and reported by the validator (against 17.1.2 "component_1 must not equal component_2") in #304 but not prevented from happening elsewhere.

[hsorby]

We don't close the issue until the code in this repository actually has the fix. That is to say once the PR that addresses this issue is merged.

[kerimoyle]

Validator checks this now in PR #355