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

Regression in EXCEPT handling caused by recent commit #134

Closed
chrispcampbell opened this issue Oct 12, 2021 · 1 comment · Fixed by #125
Closed

Regression in EXCEPT handling caused by recent commit #134

chrispcampbell opened this issue Oct 12, 2021 · 1 comment · Fixed by #125
Assignees
Milestone

Comments

@chrispcampbell
Copy link
Contributor

chrispcampbell commented Oct 12, 2021

@ToddFincannon Sorry for not catching this sooner, but apparently when I tested your fix for #117 using En-ROADS, I hadn't properly npm linked SDE, so I was using the wrong version of SDE.

The following construct in En-ROADS worked with 7c51641 (corresponds to PR #116), but started failing to compile with d92343e (corresponds to PR #118):

Elec Paths : ECoal,ECoal CCS,EOil,EOil CCS,EGas,EGas CCS,EBio,EBio CCS,nuclear,hydro,\
		Wind,Solar,Geothermal,Other renew,new
	~	
	~		|

Renewable types : Wind,Solar,Geothermal,Other renew
	~	
	~		|

Storage Cost[Renewable types]  = 
        Pre breakthrough cost for storage 
             * ( 1
                  - Post breakthrough effect for storage )  ~~|

Storage Cost[Elec Paths] :EXCEPT: [Renewable types] = 0
	~	$/GJ
	~	Marginal storage cost, only applicable for renewables.
	|

Running sde generate --genc produces the following error:

ERROR: no var found for refId : '_pre_breakthrough_cost_for_storage'

ERROR: no var found for refId : '_post_breakthrough_effect_for_storage'

TypeError: Cannot read property 'varType' of undefined
    at propEq (/Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/propEq.js:38:25)
    at /Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_curry3.js:39:18
    at f1 (/Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_curry1.js:19:17)
    at _filter (/Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_filter.js:7:9)
    at /Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/filter.js:69:3
    at /Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_dispatchable.js:50:15
    at Object.f2 [as filter] (/Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_curry2.js:34:14)
    at refs (file:///Users/campbell/Projects/CI/SDEverywhere-alt3/src/Model.js:724:21)
    at file:///Users/campbell/Projects/CI/SDEverywhere-alt3/src/Model.js:718:35
    at _map (/Users/campbell/Projects/CI/SDEverywhere-alt3/node_modules/ramda/src/internal/_map.js:7:19)

I think there are actually two bugs here, or rather there are two symptoms that might be traced back to the same bug. I pushed some standalone tests that reproduce the issue to the chris/134-except-regression branch.

  1. If you run the except2 test, it reproduces the issue above ("no var found for refId") in a standalone test case. This one uses a spec.json file to specify a subset of outputs.

  2. If you run the modified except test, it seems to calculate z total incorrectly and results in differences. This one does not use a spec.json file. The generated C code in this case looks like this:

  // z total = SUM(z[SubA!])
  double __t1 = 0.0;
	for (size_t v = 0; v < 2; v++) {
	  __t1 += _z[_suba[v]];
	}
  _z_total = __t1;
  // z[SubA] = z ref a*z ref b
  _z[1] = _z_ref_a * _z_ref_b;
  // z[SubA] = z ref a*z ref b
  _z[2] = _z_ref_a * _z_ref_b;

My guess is that we're not setting some references when the variable is expanded so the dependency chain is not computed correctly (and dependents may be dropped during the dead code removal phase as a result), but I haven't investigated this too deeply.

@ToddFincannon
Copy link
Collaborator

Tracing through this I saw incorrect behavior on your new z variable that involves both a subdimension and EXCEPT clause. The tests in the chris/134-except-regression branch pass when I used the todd/124-const-subdimension branch that fixed the subdimension/EXCEPT case. Could we try pressing on with the PR merges that far to see if it fixes the regression for you too?

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

Successfully merging a pull request may close this issue.

2 participants