Analyser: reworked the analysis of a model with external variables #1077
Conversation
By decrementing things wherever possible, which makes the code easier to read and, therefore, to update/maintain.
We used to do handle them after having analysed the model which meant having to requalify a variable-based constant equation as an algebraic equation if it depended on one or several external variables. In other words, as a developer, we had to think about what consequence(s) an external variable might have on our analysed model. Until now, it wasn't too much a problem, but with our wish to support systems of non-linear algebraic equations (and then DAEs), this is going to become a nightmare.
... since we may have only generated warnings.
With:
```
mDependencies.erase(std::remove_if(mDependencies.begin(), mDependencies.end(),
[=](const auto &dependency) -> bool {
return dependency.lock()->variable() == nullptr;
}),
```
it reports that the first line is not covered...!?
Even with:
```
mDependencies.erase(std::remove_if(mDependencies.begin(), mDependencies.end(), [=](const auto &dependency) -> bool {
return dependency.lock()->variable() == nullptr;
}),
mDependencies.end());
```
GCov reports that the first line is not covered...!? So, we now put everything on stupidly long line!
|
I will likely have another quick look at it tomorrow morning, but I believe this is ready to be reviewed. This PR:
The second point is really the reason behind this PR since right now an equation of type With this PR, by handling external variables as early as possible, we do NOT need to do any requalification anymore (see here). This is going to be particularly useful when addressing issue #882 since external variables could otherwise make things rather tricky. |
|
When nothing actually needs to be changed to get the CI to pass, you should be able to just click the 're-run failed tests' button; you don't need a whole dummy commit. Just FYI; I went through that phase myself before figuring it out. |
Agreed when it comes to GitHub Actions. I have done it on occasions, but we are not using GH Actions for our CI. We use Buildbot and, yesterday, I tried to rerun one of the tests and it passed, but its updated status wasn't reflected in GitHub. So, I had to create a dummy commit and hope for the best. Anyway, will see what @hsorby has to say about it. All I know is that it's very frustrating and a waste of people's time. |
There was a problem hiding this comment.
Assuming CI gets back to showing this as passing, then it looks good to me. I did wonder if there needed to be an update to the docs to go along with this change, but then I found: https://libcellml.org/documentation/v0.4.0/user/asides/analyser#understanding-analyser, so that will be a future issue ;-)
🤪 |
Fixes issue #1074.