fix: use original Vensim names in generated headers for spec output vars#462
Conversation
|
|
||
| const char* getHeader() { | ||
| return "${R.map(varName => headerTitle(varName), headerVars).join('\\t')}"; | ||
| return "${headerVars.join('\\t')}"; |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
|
@chrispcampbell I made the changes to the test models we discussed and found one other bug in the process. It's ready for you to review again. |
… is accessed from user's spec file
ToddFincannonEI
left a comment
ToddFincannonEI
left a comment
There was a problem hiding this comment.
Thanks for finding all the other references to outputVars and clarifying the other references to outputVarNames. Much improved!
chrispcampbell
left a comment
chrispcampbell
left a comment
There was a problem hiding this comment.
@ToddFincannonEI: Looks good, thanks, Todd. I made a couple small tweaks to comments/docs which I noted in inline comments, the rest looked good so I will merge now.
| } else { | ||
| outputAllVars = true | ||
| } | ||
| let outputAllVars = spec.outputVarNames === undefined || spec.outputVarNames.length === 0 |
There was a problem hiding this comment.
I simplified this logic now that the user can only provide outputVarNames and outputVars is only used internally.
| let headerVars = outputAllVars ? expandedVarNames(true) : spec.outputVars | ||
| let outputVars = outputAllVars ? expandedVarNames() : spec.outputVars | ||
| let headerVarNames = outputAllVars ? expandedVarNames(true) : spec.outputVarNames | ||
| let outputVarIds = outputAllVars ? expandedVarNames() : spec.outputVars |
There was a problem hiding this comment.
I renamed these two variables to emphasize the difference, but otherwise it's the same as you had it.
| // convert those to C names first. Otherwise, use `input/outputNames` which are already | ||
| // assumed to be valid C names. | ||
| // If the spec file contains `input/outputVarNames`, convert the full Vensim variable | ||
| // names to C names first so that later phases only need to work with canonical names |
There was a problem hiding this comment.
I updated this comment to make it match the new reality better (also input/outputNames had the wrong property names anyway).
| "inputVars": ["Reference predators", "Reference prey"], | ||
| "outputVars": ["Time", "Predators Y", "Prey X"] | ||
| "inputVarNames": ["Reference predators", "Reference prey"], | ||
| "outputVarNames": ["Time", "Predators Y", "Prey X"] |
There was a problem hiding this comment.
I updated the README (which I guess had been incorrect for a while) so that it only shows using input/outputVarNames.
2 participants
Fixes #461
See issue #461 for details.