Skip to content

Generator: computeVariables() recomputes what computeRates() has just computed #1481

Open
#1482
Open
Generator: computeVariables() recomputes what computeRates() has just computed#1481
#1482

Description

@agarny
agarny
opened on Sep 29, 2026

Note: this assumes that PR #1473 has been merged in.

computeVariables() is to be computed every time a model is integrated, so that variables that depend on states are up to date. However, some variables may also depend on rates and rates themselves may depend on states. The bottom line is that, once a model has been integrated, we should call both computeRates() and computeVariables().

Now, the issue is that computeVariables() recomputes most of what computeRates() has just computed while it should only compute what wasn't computed by computeRates(). So, we end up computing the same thing twice for no good reason, i.e. waste of computing time.

Example (k0 is a constant)

k = 2*k0
a = sin(t)
c = k*t
b = a*x
d = b+ode(x, t)+c
ode(z, t) = d-z
ode(x, t) = -(k*x+b)

computeRates() computes a, b, c and d, with rates[0] computed before d:

algebraicVariables[1] = computedConstants[0]*voi;                              // c
algebraicVariables[0] = sin(voi);                                              // a
algebraicVariables[2] = algebraicVariables[0]*states[0];                       // b
rates[0] = -(computedConstants[0]*states[0]+algebraicVariables[2]);
algebraicVariables[3] = algebraicVariables[2]+rates[0]+algebraicVariables[1];  // d
rates[1] = algebraicVariables[3]-states[1];

Yet computeVariables() recomputes all four of them:

algebraicVariables[0] = sin(voi);                                              // a
algebraicVariables[2] = algebraicVariables[0]*states[0];                       // b
algebraicVariables[1] = computedConstants[0]*voi;                              // c
algebraicVariables[3] = algebraicVariables[2]+rates[0]+algebraicVariables[1];  // d

Adding an unrelated equation, r = cos(t), means that a and c (now algebraicVariables[1] and algebraicVariables[2]) are no longer recomputed, but b and d still are:

algebraicVariables[0] = cos(voi);                                              // r
algebraicVariables[3] = algebraicVariables[1]*states[0];                       // b
algebraicVariables[4] = algebraicVariables[3]+rates[0]+algebraicVariables[2];  // d

Only r needs computing here.

Proposal

  1. Document that computeVariables() requires computeRates() to have been executed first, at the same point.
  2. Generate computeVariables() without the equations that computeRates() computes, so that it only computes the variables that computeRates() doesn't need (r in the second example).

Activity

  1. nickerso commented on Sep 30, 2026

    @nickerso
    nickerso
    on Sep 30, 2026
    Contributor

    avoiding duplication seems good. As long as its clear to users that computeRates() should always be called before calling computeVariables() then that seems ok to me. I'd probably expect the name of the method to change, but not sure what to - computeNonStateVariables() or computeNonRateDependentVariables() doesn't sound good :)

    I do, however, like the idea of a single computeVariables() method being generated that would give me a single call to update all variables in a model to correspond to the current state and also mean not having to come up with a new method name.

  2. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    avoiding duplication seems good. As long as its clear to users that computeRates() should always be called before calling computeVariables() then that seems ok to me.

    That's the idea: mention in the API documentation that computeRates() must be called before calling computeVariables().

    I'd probably expect the name of the method to change, but not sure what to - computeNonStateVariables() or computeNonRateDependentVariables() doesn't sound good :)

    Yeah, I was also thinking of changing the name, but couldn't come up with a good alternative either. :/

    I do, however, like the idea of a single computeVariables() method being generated that would give me a single call to update all variables in a model to correspond to the current state and also mean not having to come up with a new method name.

    But that single computeVariables() method would be the same as calling computeRates() and the 'new' computeVariables()? If so, it would mainly be a convenience method, right? If so, yes, it might be nice, but this would generate more code just for the sake of convenience. So, not sure I am too keen.

  3. nickerso commented on Sep 30, 2026

    @nickerso
    nickerso
    on Sep 30, 2026
    Contributor

    But that single computeVariables() method would be the same as calling computeRates() and the 'new' computeVariables()? If so, it would mainly be a convenience method, right? If so, yes, it might be nice, but this would generate more code just for the sake of convenience. So, not sure I am too keen.

    Convenience for users is important. Generating a little duplicated code makes no difference to the generator.

  4. hsorby commented on Sep 30, 2026

    @hsorby
    hsorby
    on Sep 30, 2026
    Contributor

    Does the computeVariables() funciton only update algebraic variables? Is that the right name for that function?

  5. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    Does the computeVariables() funciton only update algebraic variables? Is that the right name for that function?

    computeVariables() computes both algebraic and external variables, incl. through an NLA system.

    Convenience for users is important. Generating a little duplicated code makes no difference to the generator.

    At this point, I would have computeRates(), computeVariables(), and if really needed computeRatesAndVariables().

  6. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    computeRatesAndVariables() would just be a wrapper around computeRates() and computeVariables().

  7. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    Maybe for another issue? If so, feel free to create that issue @nickerso.

  8. hsorby commented on Sep 30, 2026

    @hsorby
    hsorby
    on Sep 30, 2026
    Contributor

    At this point, I would have computeRates(), computeVariables(), and if really needed computeRatesAndVariables().

    Maybe this just becomes computeUpdate()?

  9. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    At this point, I would have computeRates(), computeVariables(), and if really needed computeRatesAndVariables().

    Maybe this just becomes computeUpdate()?

    You mean computeUpdate() instead of computeRatesAndVariables()? I am not a fan of computeUpdate(). Doesn't tell me what is updated. It could be anything. That's why we have computeRates() and computeVariables() in the first place. We know what we are computing.

  10. nickerso commented on Sep 30, 2026

    @nickerso
    nickerso
    on Sep 30, 2026
    Contributor

    Maybe for another issue? If so, feel free to create that issue @nickerso.

    But discussing how to best address this issue is what this issue is about, right?

    computeRatesAndVariables() would just be a wrapper around computeRates() and computeVariables().

    I'm thinking there would be computeRates() which is what a user would use with their integrator and computeVariables() would just have all the code in it to update all variables in the model to the current state.

    But as I mentioned before, as long as computeVariables() has a relevant name and the documentation is clear on how the generated methods should be used, then that would be acceptable. I don't think its acceptable to have a method generated call computeVariables() when that is not what the method does.

  11. agarny commented on Sep 30, 2026

    @agarny
    agarny
    on Sep 30, 2026
    ContributorAuthor

    Maybe for another issue? If so, feel free to create that issue @nickerso.

    But discussing how to best address this issue is what this issue is about, right?

    Sure, of course!

    computeRatesAndVariables() would just be a wrapper around computeRates() and computeVariables().

    I'm thinking there would be computeRates() which is what a user would use with their integrator and computeVariables() would just have all the code in it to update all variables in the model to the current state.

    But as I mentioned before, as long as computeVariables() has a relevant name and the documentation is clear on how the generated methods should be used, then that would be acceptable. I don't think its acceptable to have a method generated call computeVariables() when that is not what the method does.

    I believe the API documentation mentions what it is about. But, at this stage, I am happy to go with whatever. Just need to be told what.

    In the meantime, I shall use PR #1482 in [lib]OpenCOR (together with PR #1473, PR #1475, and PR #1477).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions