Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect initial model outputs when SAVEPER = TIME STEP #253

Closed
chrispcampbell opened this issue Sep 29, 2022 · 0 comments · Fixed by #254 or #255
Closed

Incorrect initial model outputs when SAVEPER = TIME STEP #253

chrispcampbell opened this issue Sep 29, 2022 · 0 comments · Fixed by #254 or #255
Assignees

Comments

@chrispcampbell
Copy link
Contributor

This issue only applies to the runModelWithBuffers entrypoint that is used by the @sdeverywhere/runtime package.

The problem is that some (many?) Vensim models have SAVEPER set to TIME STEP rather than a constant value (typically 1). In the case where it's a constant, then there's no problem. But when it is set to TIME STEP, then it is treated like any other "aux" variable, which means its value isn't computed in initConstants but rather in evalAux. But we have code that initializes numSavePoints before that first call to evalAux, which means that it's getting set to the wrong value for the initial model run, which causes incorrect outputs.

The fix (for now) is to move the initialization of this value to just after the first evalAux:

  while (step <= lastStep) {
    evalAux();
    if (fmod(_time, _saveper) < 1e-6) {
      // Note that many Vensim models set `SAVEPER = TIME STEP`, in which case SDE
      // treats `SAVEPER` as an aux rather than a constant.  Therefore, we need to
      // initialize `numSavePoints` here, after the first `evalAux` call, to be
      // certain that `_saveper` has been initialized before it is used.
      if (numSavePoints == 0) {
        numSavePoints = (size_t)(round((_final_time - _initial_time) / _saveper)) + 1;
      }
      outputVarIndex = 0;
      storeOutputData();
      savePointIndex++;
    }

I won't be including a test case for this with this PR but tests that exercise this case will be added soon (as part of a set of integration tests that exercise the runtime and runtime-async packages).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant