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: initialize control parameters prior to access #304

Merged
merged 3 commits into from
Dec 13, 2022

Conversation

chrispcampbell
Copy link
Contributor

Fixes #301

With this change we no longer generate the control param accessors as part of code gen, but instead define accessors in model.c that return the computed values (e.g., _initial_time). For this to work correctly in cases where the model's control parameters are computed (or initialized based on external data files), we run the model for the first time step only, and we do this lazily on first use of the accessors.

No change in behavior for models that use constant values for the control params. This fix mainly applies to models that define the control params in an external file, like EPS.

I added a JS-level integration test that verifies the behavior for the latter case where they are defined in an external csv file. For this test, we actually cover a mix of external and computed params:

  • INITIAL TIME and TIME STEP are defined in a csv file
  • FINAL TIME is computed based on INITIAL TIME
  • SAVEPER is computed based on TIME_STEP

@chrispcampbell
Copy link
Contributor Author

@ToddFincannonEI: If you can review the cli/compile changes, that would be great. The new integration test is mostly copy/pasted from the previous saveper test, so you don't need to worry as much about those changes. (I'd like to simplify the JS-level integration tests eventually to not require so much boilerplate, but I'll worry about that later once we have more of them.)

Copy link
Collaborator

@ToddFincannonEI ToddFincannonEI left a comment

Choose a reason for hiding this comment

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

I'm wondering if initConstants, initLevels, and evalAux would be called twice — here in the first call to initControlParameters, and then in runModel. I am pretty sure those calls are idempotent, but it could amount to a lot of run time in a large model. I might be missing how this works.

I also don't set setInputs getting called in initControlParameters, so I don't understand how inputs could affect the control parameters.

@chrispcampbell
Copy link
Contributor Author

I'm wondering if initConstants, initLevels, and evalAux would be called twice — here in the first call to initControlParameters, and then in runModel. I am pretty sure those calls are idempotent, but it could amount to a lot of run time in a large model. I might be missing how this works.

Yes, those will run once (and only once) as part of initControlParameters, so the first time they're called in run will be redundant.

My assumption is that even for a large model, the cost of running these functions once at init time is relatively tiny. Using En-ROADS as an example, a single model run includes ~880 time steps and runs in about 20 ms, so this init code might take around 0.02 ms. There are probably other larger models where it could matter more, but my thinking is that this fix is more of a stopgap just to get us past the current regression affecting EPS, and we can improve upon it later.

I considered other alternative approaches that would avoid the redundancy. For example:

  • We could make code gen produce a separate init function that includes the subset of initConstants and evalAux that are needed to initialize the control params without dragging in all the rest.
  • We could change run to skip the first initConstants if we know that it was already done as part of the initControlParams (but that won't save much time).
  • We could go back to making the user/developer have to provide values at config/init time (error prone and not very friendly).

But I think all of those are more involved than the simplest fix we can do for now.

I also don't set setInputs getting called in initControlParameters, so I don't understand how inputs could affect the control parameters.

That TODO comment I included is referring to the fact that we don't factor in runtime inputs there (like we do for the normal run function). Maybe I could've worded the comment more clearly. I think the more important thing (since TODO comments are overlooked) is that I added this deficiency as a comment in #302 to be considered later.

To put it another way, I think we agreed to punt for now on handling the possibility of a model whose control parameters are influenced by inputs provided at runtime, correct?

@ToddFincannonEI
Copy link
Collaborator

Thanks for the explanation. I thought you had refactored to support control parameter inputs, but I see now that has been deferred. The extra init calls are a bigger hit for EPS, which usually only has 30 time steps, but initializes a huge number of variables generated from data files. However, I don't think it will affect EPS, because we are not using the SDEverywhere runtime yet. So I'm fine with completing this change as written.

@chrispcampbell
Copy link
Contributor Author

OK, thanks for the review. I'll merge these changes shortly to resolve the regression, but I've filed a separate issue #305 to revisit this fix and I'll try to come up with a less heavy-handed approach to initializing these parameters.

@chrispcampbell chrispcampbell merged commit 365bbb2 into main Dec 13, 2022
@chrispcampbell chrispcampbell deleted the chris/301-control-params branch December 13, 2022 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Control parameter accessors need to generate code
2 participants