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

fix: allow extra index subscripts in 2D const lists #110

Merged
merged 8 commits into from
Sep 27, 2021

Conversation

ToddFincannon
Copy link
Collaborator

@ToddFincannon ToddFincannon commented Sep 15, 2021

Fixes #109

This code that was introduced in EquationGen in 2ed7e42 for PR #101 (issue #84):

for (let i = 0; i < this.var.separationDims.length; i++) {
  const dim = this.var.separationDims[i]
  const sepDim = sub(dim)
  const ind = this.var.subscripts[i]

is incorrect in this case because the index i is applied to both the separationDims and subscripts arrays. When the separationDims are a proper subset of subscripts, their elements do not correspond.

It turns out we don't need to look up anything. The array variable that will be initialized from the const list is already separated into individually indexed elements at this point. We simply have to convert the canonical index names to numerical C indices to look up the variable's position in the const list. Even better, this works for both one- and two-dimensional arrays.

The other problem that needed to be fixed was in VariableReader. It assumed that the subscripts only included the separation dimensions. When an additional index subscript is present, as in this case, the separated variables created by VariableReader would get an incomplete set of subscripts. While fixing this, I was also able to simplify the code by replacing the doubly nested loops with iteration over the cartesian product of the subscript dimensions. This could probably be applied to the 1D case as well, but that code was working with the additional index subscript, so I left it alone.

This fix requires the fix for antlr4-vensim issue climateinteractive/antlr4-vensim#7.

I added a new equation to the arrays_cname test model to cover the case for this issue.

@ToddFincannon ToddFincannon added this to the 0.6.0 milestone Sep 15, 2021
Base automatically changed from todd/29-delay-fixed to develop September 27, 2021 20:52
Copy link
Contributor

@chrispcampbell chrispcampbell left a comment

Choose a reason for hiding this comment

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

@ToddFincannon This looks good to me. Noting for the record that the VariableReader code will be further improved by PR #118 which will be handled in the near future. I think it would be good if we could add a couple more variations on the test you added here (to test different combinations, like [DimA, C1, DimB]) but I think I will suggest that in #118 so as to not hold this one up (to help break the PR log jam). I'll merge this shortly.

@chrispcampbell chrispcampbell merged commit f5494af into develop Sep 27, 2021
@chrispcampbell chrispcampbell deleted the todd/109-const-list branch September 27, 2021 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Code generation fails for 2D constant lists with an LHS index
2 participants