search

cellml / libcellml

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

Analyser: reworked the analysis of a model with external variables #1077

Closed agarny closed 1 year ago

agarny commented 1 year ago

Fixes issue #1074.

agarny commented 1 year ago

I will likely have another quick look at it tomorrow morning, but I believe this is ready to be reviewed.

This PR:

  1. improves the handling of external variables in the analyser, as well as
  2. moves that handling as early as possible in the analysis of a model.

The second point is really the reason behind this PR since right now an equation of type VARIABLE_BASED_CONSTANT has to be requalified as an ALGEBRAIC equation if it relies on external variables (see here).

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.

agarny commented 1 year ago

Ok, just a minor cleaning up of the code and various dummy commits that eventually resulted in the CI passing as its should have in the first place (what a waste of time!). So, now, all good to review. @hsorby and @nickerso.

luciansmith commented 1 year ago

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.

agarny commented 1 year ago

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.

agarny commented 1 year ago

@hsorby and @nickerso: would be nice to have this one reviewed (and then PR #1080).

agarny commented 1 year ago

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 ;-)

🤪