cellml / libcellml

Repository for libCellML development.
https://libcellml.org
Apache License 2.0
17 stars 21 forks source link

Analyser: add support for unknown variables that have been marked as external #1184

Closed agarny closed 1 year ago

agarny commented 1 year ago

Fixes #1182.

agarny commented 1 year ago

@hsorby and @nickerso: Weiwei said that she could get our test case to work without this fix by initialising that P_3 parameter. So, one might think that we could consider all the variables that have been marked as external as being initialised, i.e. something like:

for (const auto &internalVariable : mInternalVariables) {
    if (internalVariable->mIsExternal) {
        internalVariable->mType = AnalyserInternalVariable::Type::INITIALISED;
    }
}

...

do {
    relevantCheck = false;
    for (const auto &internalEquation : mInternalEquations) {
        relevantCheck = internalEquation->check(mModel, stateIndex, variableIndex, checkNlaSystems)
                        || relevantCheck;
    }

    if (!relevantCheck && !checkNlaSystems) {
        relevantCheck = true;
        checkNlaSystems = true;
    }
} while (relevantCheck);

but this will fail for more "complicated" cases (e.g., TEST(Generator, hodgkinHuxleySquidAxonModel1952WithComputedConstantAsExternalVariable)). So, the fact that Weiwei's fix works for our test case is just a coincidence and we need the current fix to account for the general case.

Anyway, it's now ready for review.

agarny commented 1 year ago

I think the documentation should mention that a variable marked as external is the responsibility of the user. It should also be made clear that an external variable is considered as initialised and again it is the responsibility of the user to make sure relevant conditions for its use are met.

My concern is if we don't do it now we will forget about it and I feel it is rather an important piece of information for users to have.

Fair point: 4d28f2a0.