Skip to content

fix: use original Vensim names in generated headers for spec output vars #462

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.

Sign up for GitHub

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

Merged
chrispcampbell merged 10 commits into from
Apr 22, 2024
Merged

fix: use original Vensim names in generated headers for spec output vars #462

Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f97985b
use original Vensim names in generated headers for spec output vars
ToddFincannonEI Apr 18, 2024
bc31a87
remove obsolete arrays_cname test model
ToddFincannonEI Apr 19, 2024
821d8a1
rename arrays_varname test model to arrays
ToddFincannonEI Apr 19, 2024
94edffc
use Vensim format outputVarNames in all test model spec files
ToddFincannonEI Apr 19, 2024
07e36fe
escape double quotes in header variable names
ToddFincannonEI Apr 20, 2024
3005eb0
update arrays test model name in the compile tests
ToddFincannonEI Apr 20, 2024
39172e1
Merge branch 'main' into todd/461-validation-subs
ToddFincannonEI Apr 20, 2024
aa85254
fix: update comment to better reflect how input/outputVarNames are co…
chrispcampbell Apr 22, 2024
9f23719
fix: simplify and clarify code gen logic now that only outputVarNames…
chrispcampbell Apr 22, 2024
f5f8625
fix: update README for cli package to show correct usage of input/out…
chrispcampbell Apr 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions packages/compile/src/generate/code-gen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,15 +157,15 @@ ${chunkedFunctions('evalLevels', Model.levelVars(), ' // Evaluate levels.')}
// Input/output section
//
function emitIOCode() {
let headerVars = outputAllVars ? expandedVarNames(true) : spec.outputVars
let headerVars = outputAllVars ? expandedVarNames(true) : spec.outputVarNames
let outputVars = outputAllVars ? expandedVarNames() : spec.outputVars
mode = 'io'
return `void setInputs(const char* inputData) {${inputsFromStringImpl()}}

void setInputsFromBuffer(double* inputData) {${inputsFromBufferImpl()}}

const char* getHeader() {
return "${R.map(varName => headerTitle(varName), headerVars).join('\\t')}";
return "${headerVars.join('\\t')}";
Copy link
Copy Markdown
Contributor

@chrispcampbell chrispcampbell Apr 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ToddFincannonEI: It looks like the build is failing due to a test failure when running the "allocate" model, but probably others are affected as well:
https://github.com/climateinteractive/SDEverywhere/actions/runs/8741036880/job/23986139008

The error points back to this line, which will fail if headerVars is undefined.

The issue is that there are two ways to specify output variables in the spec.json, you can either use outputVars (which takes C names) or outputVarNames (which takes Vensim names). In the case of outputVarNames, we convert to outputVars in this code in Model.read so that the rest of the code only needs to deal with outputVars (sort of a lowest common denominator):

  if (spec) {
    // If the spec file contains `input/outputVarNames` (with full Vensim variable names)
    // convert those to C names first.  Otherwise, use `input/outputNames` which are already
    // assumed to be valid C names.
    if (spec.inputVarNames) {
      spec.inputVars = R.map(cName, spec.inputVarNames)
    }
    if (spec.outputVarNames) {
      spec.outputVars = R.map(cName, spec.outputVarNames)
    }
    // Save the input vars locally so that they can be referenced by `isInputVar`.
    if (spec.inputVars) {
      inputVars = spec.inputVars
    }
  }

There are only 3 sample models that use the older outputVars form (allocate, arrays_cname, longeqns).

There are a few different ways we could solve this. I don't think it's imperative that we retain support for outputVars. For En-ROADS/C-ROADS we use the newer outputVarNames and all the new build stuff in SDE also uses that form, so I doubt anyone out in the wild is relying on outputVars. I think if we changed those 3 tests to use outputVarNames and effectively drop support for outputVars, then your proposed fix will be nearly correct (though I'd need to look through the rest of the code to see if any other changes are needed).

Let me know which approach you'd like to take.

Copy link
Copy Markdown
Collaborator Author

@ToddFincannonEI ToddFincannonEI Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forgot to run the tests before submitting. EPS uses outputVarNames too, so your proposed solution looks good to me. I will change the tests and resubmit.

Copy link
Copy Markdown
Collaborator Author

@ToddFincannonEI ToddFincannonEI Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the only purpose of the arrays_cname test model is to test the outputVars C name support in spec files. I propose deleting this model, since we are dropping support for outputVars.

Copy link
Copy Markdown
Contributor

@chrispcampbell chrispcampbell Apr 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ToddFincannonEI: I agree with deleting arrays_cname, and also suggest renaming arrays_varname back to just arrays, but I'd be OK handling that myself in a separate PR after this one.

}

void storeOutputData() {
Expand Down Expand Up @@ -382,9 +382,6 @@ ${postStep}
}
return inputVars
}
function headerTitle(varName) {
return Model.vensimName(varName).replace(/"/g, '\\"')
}

return {
generate: generate
Expand Down