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

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

Closed
ToddFincannon opened this issue Sep 14, 2021 · 1 comment · Fixed by #110 or #190
Closed

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

ToddFincannon opened this issue Sep 14, 2021 · 1 comment · Fixed by #110 or #190
Assignees
Labels
Milestone

Comments

@ToddFincannon
Copy link
Collaborator

Two-dimensional constant lists with an index on the LHS do not generate code correctly. For instance, this model:

DimA: A1, A2, A3 ~~|
DimB: B1, B2 ~~|
DimC: C1, C2 ~~|

p[DimA, DimB, C1]=
0, 0;
0, 0;
0, 0;
~~|

emits the following error message when generating C code:

_p[_a1,_b1] → _p[0][0] not found in C names

The LHS only contains the dimension subscripts, thus the search through C names that contain all three subscripts fails. Constant lists are separated into individual equations that assign one constant value to each combination of subscripts resolved into indices. The code generator assumes that only the separation dimensions are present on the LHS.

@ToddFincannon ToddFincannon added this to the 0.6.0 milestone Sep 14, 2021
@ToddFincannon ToddFincannon self-assigned this Sep 14, 2021
@ToddFincannon
Copy link
Collaborator Author

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 #7.

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

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