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 code generated when multiple subscripts resolve to same family #278

Closed
chrispcampbell opened this issue Oct 28, 2022 · 3 comments · Fixed by #588
Closed

Incorrect code generated when multiple subscripts resolve to same family #278

chrispcampbell opened this issue Oct 28, 2022 · 3 comments · Fixed by #588
Assignees

Comments

@chrispcampbell
Copy link
Contributor

chrispcampbell commented Oct 28, 2022

We have a model that has two dimensions with different names that resolve to the same subscript family. Currently SDE produces incorrect results for equations that involve these dimensions when there are 3 subscripts involved on the LHS.

SDE has existing code to deal with similar cases involving 2 subscripts, see compile/src/_shared/subscript.js, but that code as written only handles the case where the LHS has 2 subscripts and the RHS has 2 subscripts. We need to generalize that code so that it works with >= 2 subscripts on either LHS or RHS.

Here's an isolated test case that demonstrates the issue. I will push this to a branch under models/subfamilies.

Scenario : S1, S2
  ~~|

Sector : A1, A2, A3
  ~~|

Supplying Sector : A1, A2
  -> Producing Sector
  ~~|

Producing Sector : A1, A2
  -> Supplying Sector
  ~~|

X[A1,A1] = 101 ~~|
X[A1,A2] = 102 ~~|
X[A1,A3] = 103 ~~|
X[A2,A1] = 201 ~~|
X[A2,A2] = 202 ~~|
X[A2,A3] = 203 ~~|
X[A3,A1] = 301 ~~|
X[A3,A2] = 302 ~~|
X[A3,A3] = 303 ~~|

Y[S1] = 1000 ~~|
Y[S2] = 2000 ~~|

Z[Scenario, Supplying Sector, Producing Sector] = 
  Y[Scenario] + X[Supplying Sector, Producing Sector]
  ~
  ~
  ~ :SUPPLEMENTARY 
  |

This produces the following differences (compounded by the confusion that occurs when comparing variables that have multiple instances of the same subscript/dimension, as noted in the part about normalizeSubscripts in the evaluation of #179):

_u[_a1,_a2] time=0.00 vensim=107 sde=106 diff=0.934579%
_u[_a1,_a2] time=1.00 vensim=107 sde=106 diff=0.934579%
_u[_a2,_a1] time=0.00 vensim=206 sde=207 diff=0.485437%
_u[_a2,_a1] time=1.00 vensim=206 sde=207 diff=0.485437%
_v[_s1,_a1,_a2] time=0.00 vensim=1102 sde=1101 diff=0.090744%
_v[_s1,_a1,_a2] time=1.00 vensim=1102 sde=1101 diff=0.090744%
_v[_s1,_a2,_a1] time=0.00 vensim=1201 sde=1202 diff=0.083264%
_v[_s1,_a2,_a1] time=1.00 vensim=1201 sde=1202 diff=0.083264%
_v[_s2,_a1,_a2] time=0.00 vensim=2102 sde=2101 diff=0.047574%
_v[_s2,_a1,_a2] time=1.00 vensim=2102 sde=2101 diff=0.047574%
_v[_s2,_a2,_a1] time=0.00 vensim=2201 sde=2202 diff=0.045434%
_v[_s2,_a2,_a1] time=1.00 vensim=2201 sde=2202 diff=0.045434%

This is similar to #179, but as far as I can tell, there are different underlying issues. (My proposed fix for this issue doesn't seem to help with #179.)

@chrispcampbell
Copy link
Contributor Author

chrispcampbell commented Nov 25, 2024

This bug is fixed by the new implementation of the expandedRefIdsForVar function that correctly handles situations like this where two dimensions are used that refer back to the same family of subscripts.

I added the following test model in read-equations.spec.ts (and similar ones in gen-equation-{c,js}.spec.ts), which is basically the same as the test model in the issue description above.

Prior to the fix, this test would fail on the lines marked "BUG". After the fix, the correct refIds are generated and the test passes.

  describe('when LHS is NON-apply-to-all (3D)', () => {
    // ...

    // This test is based on the example from #278
    it('should work when RHS variable is NON-apply-to-all (2D) and is accessed with 2 different dimensions from LHS that map to the same family', () => {
      const vars = readInlineModel(`
        Scenario: S1, S2 ~~|
        Sector: A1, A2, A3 ~~|
        Supplying Sector: A1, A2 -> Producing Sector ~~|
        Producing Sector: A1, A2 -> Supplying Sector ~~|
        x[A1,A1] = 101 ~~|
        x[A1,A2] = 102 ~~|
        x[A1,A3] = 103 ~~|
        x[A2,A1] = 201 ~~|
        x[A2,A2] = 202 ~~|
        x[A2,A3] = 203 ~~|
        x[A3,A1] = 301 ~~|
        x[A3,A2] = 302 ~~|
        x[A3,A3] = 303 ~~|
        y[S1] = 1000 ~~|
        y[S2] = 2000 ~~|
        z[Scenario, Supplying Sector, Producing Sector] =
          y[Scenario] + x[Supplying Sector, Producing Sector]
          ~~|
      `)
      expect(vars).toEqual([
        // refIdsForRhsVarRef(_z[_scenario,_a1,_a1], '_x', ['_supplying_sector', '_producing_sector'])
        //   -> ['_x[_a1,_a1]']
        v('z[Scenario,Supplying Sector,Producing Sector]', 'y[Scenario]+x[Supplying Sector,Producing Sector]', {
          refId: '_z[_scenario,_a1,_a1]',
          subscripts: ['_scenario', '_a1', '_a1'],
          separationDims: ['_supplying_sector', '_producing_sector'],
          references: ['_y[_s1]', '_y[_s2]', '_x[_a1,_a1]'],
          varType: 'aux'
        }),
        // refIdsForRhsVarRef(_z[_scenario,_a1,_a2], '_x', ['_supplying_sector', '_producing_sector'])
        //   -> ['_x[_a1,_a2]']
        v('z[Scenario,Supplying Sector,Producing Sector]', 'y[Scenario]+x[Supplying Sector,Producing Sector]', {
          refId: '_z[_scenario,_a1,_a2]',
          subscripts: ['_scenario', '_a1', '_a2'],
          separationDims: ['_supplying_sector', '_producing_sector'],
          references: ['_y[_s1]', '_y[_s2]', '_x[_a1,_a2]'],  // BUG: Actual value for last element is '_x[a1,_a1]'
          varType: 'aux'
        }),
        // refIdsForRhsVarRef(_z[_scenario,_a2,_a1], '_x', ['_supplying_sector', '_producing_sector'])
        //   -> ['_x[_a2,_a1]']
        v('z[Scenario,Supplying Sector,Producing Sector]', 'y[Scenario]+x[Supplying Sector,Producing Sector]', {
          refId: '_z[_scenario,_a2,_a1]',
          subscripts: ['_scenario', '_a2', '_a1'],
          separationDims: ['_supplying_sector', '_producing_sector'],
          references: ['_y[_s1]', '_y[_s2]', '_x[_a2,_a1]'],  // BUG: Actual value for last element is '_x[a2,_a2]'
          varType: 'aux'
        }),
        // refIdsForRhsVarRef(_z[_scenario,_a2,_a2], '_x', ['_supplying_sector', '_producing_sector'])
        //   -> ['_x[_a2,_a2]']
        v('z[Scenario,Supplying Sector,Producing Sector]', 'y[Scenario]+x[Supplying Sector,Producing Sector]', {
          refId: '_z[_scenario,_a2,_a2]',
          subscripts: ['_scenario', '_a2', '_a2'],
          separationDims: ['_supplying_sector', '_producing_sector'],
          references: ['_y[_s1]', '_y[_s2]', '_x[_a2,_a2]'],
          varType: 'aux'
        })
      ])

@chrispcampbell
Copy link
Contributor Author

chrispcampbell commented Nov 25, 2024

The fixes in expandedRefIdsForVar resolved the problem with incorrect refIds being included, as shown above.

However, there is still a bug on the code gen side (related to #179) that causes the newly added test to fail in gen-equation-{c,js}.spec.ts.

Until we fix that issue, the following test will fail on the lines marked "BUG". After the fix, the correct code should be generated (with correct array indices) and the test should pass.

  describe('when LHS is NON-apply-to-all (2D)', () => {
    // ...

    // This test is based on the example from #278
    it('should work when RHS variable is NON-apply-to-all (2D) and is accessed with 2 different dimensions from LHS that map to the same family', () => {
      const vars = readInlineModel(`
        Scenario: S1, S2 ~~|
        Sector: A1, A2, A3 ~~|
        Supplying Sector: A1, A2 -> Producing Sector ~~|
        Producing Sector: A1, A2 -> Supplying Sector ~~|
        x[A1,A1] = 101 ~~|
        x[A1,A2] = 102 ~~|
        x[A1,A3] = 103 ~~|
        x[A2,A1] = 201 ~~|
        x[A2,A2] = 202 ~~|
        x[A2,A3] = 203 ~~|
        x[A3,A1] = 301 ~~|
        x[A3,A2] = 302 ~~|
        x[A3,A3] = 303 ~~|
        y[S1] = 1000 ~~|
        y[S2] = 2000 ~~|
        z[Scenario, Supplying Sector, Producing Sector] =
          y[Scenario] + x[Supplying Sector, Producing Sector]
          ~~|
      `)
      expect(vars.size).toBe(15)
      // ...
      expect(genC(vars.get('_z[_scenario,_a1,_a1]'))).toEqual([
        'for (size_t i = 0; i < 2; i++) {',
        '_z[i][0][0] = _y[i] + _x[0][0];',
        '}'
      ])
      expect(genC(vars.get('_z[_scenario,_a1,_a2]'))).toEqual([
        'for (size_t i = 0; i < 2; i++) {',
        '_z[i][0][1] = _y[i] + _x[0][1];', // BUG: RHS is generated as `_x[0][0]`
        '}'
      ])
      expect(genC(vars.get('_z[_scenario,_a2,_a1]'))).toEqual([
        'for (size_t i = 0; i < 2; i++) {',
        '_z[i][1][0] = _y[i] + _x[1][0];', // BUG: RHS is generated as `_x[1][1]`
        '}'
      ])
      expect(genC(vars.get('_z[_scenario,_a2,_a2]'))).toEqual([
        'for (size_t i = 0; i < 2; i++) {',
        '_z[i][1][1] = _y[i] + _x[1][1];',
        '}'
      ])
    })
  })

@chrispcampbell
Copy link
Contributor Author

In addition to the new unit tests described above, I added a new sample model under same_family_278 using the test model from the issue description above.

This issue is effectively resolved by the same fixes that are described in my evaluation for #179.

I will submit a PR for this branch shortly and will mark both issues as being "fixed" by that PR.

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