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

Control parameter accessors need to generate code #301

Closed
ToddFincannonEI opened this issue Dec 10, 2022 · 4 comments · Fixed by #304 or #306
Closed

Control parameter accessors need to generate code #301

ToddFincannonEI opened this issue Dec 10, 2022 · 4 comments · Fixed by #304 or #306

Comments

@ToddFincannonEI
Copy link
Collaborator

I was wrong when I said that the new control parameter accessor values would always be constants, like they are in most models. EPS is highly data-driven to adapt to different model regions. The initial and final times are read from CSV files. Simply copying the formula text doesn't work. We need to generate code for the RHS after all.

The attached zip file illustrates the problem with a model adapted from EPS. Here's how the initial and final times are specified.

control.zip

INITIAL TIME=
	GET DIRECT CONSTANTS(
	   'InputData/IT.csv',
	   ',',
	   'B2*'
	)
	~	Year
	~	The initial time for the simulation.
	|

FINAL TIME=
	GET DIRECT CONSTANTS(
	   'InputData/FT.csv',
	   ',',
	   'B2*'
	)
	~	Year
	~	The final time for the simulation.
	|

This results in the following invalid C code for the new control parameter accessors.

double getInitialTime() {
  return GET DIRECT CONSTANTS('InputData/IT.csv',',','B2*');
}
double getFinalTime() {
  return GET DIRECT CONSTANTS('InputData/FT.csv',',','B2*');
}

Is it possible to define the accessors like this? Or are they not initialized yet when the runtime needs them?

double getInitialTime() {
  return _initial_time;
}
double getFinalTime() {
  return _final_time;
}
double getSaveper() {
  return _saveper;
}
@chrispcampbell
Copy link
Contributor

@ToddFincannonEI: Well, that's interesting, but I'm glad that EPS helped us to get a counterexample so soon :)

Is it possible to define the accessors like this? Or are they not initialized yet when the runtime needs them?

I started down that road when I first added the accessors, but decided against it because it would mean running some portion of the model init and running the model for one time step to properly initialize SAVEPER in the case where it's defined in terms of TIME STEP. (And now that it's clear from the EPS example that we need to be more flexible for INITIAL TIME and FINAL TIME as well.)

Looking at it again, maybe that approach wouldn't be so awful, and it would allow us to get rid of the special code gen and instead define the accessors in model.c. I'll look now and see how hard this would be.

@chrispcampbell
Copy link
Contributor

@ToddFincannonEI: I took a crack at it, preliminary changes are on a branch:
main...chris/301-control-params

The downside is that it doesn't handle the case where one or more control params are overridden by setInputs; I don't know if that's common, but it seems like a plausible scenario.

The other approach I considered is changing the way the ModelRunner implementations work in the runtime packages and make it so that the only way you can get an Outputs instance (where the control params are most needed) is by running the model once and passing undefined for the initial outputs parameter, like:

let outputs = modelRunner.runModel(inputs, undefined)

That might work, but I'll need to look at some of the call sites and in En-ROADS to see if it can pan out.

@ToddFincannonEI
Copy link
Collaborator Author

This fix works for EPS. It would be fine with me to use this and await an actual need for control params set in inputs.

@chrispcampbell
Copy link
Contributor

@ToddFincannonEI: Thanks for confirming that it works for EPS. I agree we can go with this simple solution for now. I'll submit a PR shortly.

I also opened up a new issue #302 to have a place to link possible improvements to this interface to other API improvements I'd like to make before 1.0 (so we'll do the quick thing now, and can consider how to make it better later).

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