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 nsteps bug #44

Merged
merged 2 commits into from
Oct 10, 2019
Merged

Fix nsteps bug #44

merged 2 commits into from
Oct 10, 2019

Conversation

corakingdon
Copy link
Collaborator

@corakingdon corakingdon commented Oct 7, 2019

This implements the following, which is my interpretation of the two use cases of the nsteps option to get_model:

  1. If a user wants to use the nsteps option just to shorten the time dimension, still using all the default FUND parameter values, then we need to wait to reset the time dimension until after calling set_leftover_params!, since this currently produces an error from Mimi since the default parameter values are longer than the requested time dimension.
  2. But if a user wants nsteps to be longer than the default_nsteps, then they need to provide an alternative datadir or params dictionary with values that will work for that length of time dimension. In this case, we set the new time dimension before setting the parameter values.

@corakingdon
Copy link
Collaborator Author

This will currently fail because of this test on runtests.jl line 28:

new_nsteps = 10
@test_throws ErrorException m2 = MimiFUND.get_model(nsteps = new_nsteps) #should error because parameter lenghts won't match time dim

But if we decide we want to allow this behavior, then I will delete this test and add others for the new functionality

@corakingdon corakingdon mentioned this pull request Oct 7, 2019
7 tasks
@davidanthoff davidanthoff merged commit a26f9f5 into master Oct 10, 2019
@davidanthoff davidanthoff deleted the fix-nsteps branch October 10, 2019 18:20
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.

2 participants