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

Reset parameters when dimension is changed #905

Merged
merged 10 commits into from
Jul 20, 2022
Merged

Reset parameters when dimension is changed #905

merged 10 commits into from
Jul 20, 2022

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Jun 15, 2022

This PR handles #898

@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #905 (095eea7) into master (aac4868) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #905      +/-   ##
==========================================
- Coverage   84.56%   84.56%   -0.01%     
==========================================
  Files          40       40              
  Lines        3882     3887       +5     
==========================================
+ Hits         3283     3287       +4     
- Misses        599      600       +1     
Flag Coverage Δ
unittests 84.56% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/core/dimensions.jl 89.41% <100.00%> (+0.52%) ⬆️
src/core/defcomp.jl 79.03% <0.00%> (-0.43%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aac4868...095eea7. Read the comment docs.

@lrennels
Copy link
Collaborator Author

lrennels commented Jun 28, 2022

A question here is what is the desired behavior?

When we reset :time, our solution is to map the original set parameter values to the array of new ones, and pad the rest with missing. The model is set to dirty, and upon rerun will pick up this new parameter Array. The model instance will of course hold old values until rerun. Ie.

m = Model()
set_dimension(m, :time, 2000:2005)
update_param(m, : mycomp =, :mypar, 1:6) #mypar has dimension :time
run(m)
m[:mycomp, :mypar]

# prints [1,2,3,4,5,6]

set_dimension!(m, :time, 1998:2005)
run(m)
m[:mycomp, :mypar]

# prints [missing, missing, 1,2,3,4,5,6]

Now taking a case like redefining the country dimension from ["USA", "MDV", "ALB"] to ["USA", "MDV"]. We have no promise that the new set of strings will be a subset of the old, so we cannot as easily use matching to clean up the existing parameters. One could say set_dimension!(:country, ["X", "Y", "Z"]) ... length hasn't changed but labels have.

Some ideas for if you change a (nontime) dimension are:

  1. parameters with that dimension are completely reset to nothing parameters and the user must update them again
  2. if the size of the dimension has changed, the array parameters with that dimension are reset to all missing
  3. don't automatically resize anything and assume the user will call update_param! ... but if they don't then the problem above will persist ... if they don't call update_param! which is pretty weird in terms of sizing etc.
  4. warnings of some sort

In this PR I implement (2), which is pretty consistent with how we treat time, but if we remove the _resize_parameters! part it will implement (3) simply by getting rid of the copyto! ... side note is this bad for speed? We use copyto! in updating the model instance and it seems fine to leave it there, where we are assuming parameter sizes don't change since dimensions don't change, thus this wouldn't impact Monte Carlo performance.


# check if (1) redefining dim (2) modifying a ModelDef (3) length of dimension has changed
if name!== :time && redefined && (ccd isa ModelDef) && dim_length_change
@warn("The new $name dimension keys have a different length than the original, parameters using that dimension are reset to `missings`, user will need to call `update_param!` to set them.")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will remove the warning, useful for testing.

@lrennels
Copy link
Collaborator Author

Update: we want this option: parameters with that dimension are completely reset to nothing parameters and the user must update them again

@lrennels lrennels changed the title Resize parameters when dimension is changed Reset parameters when dimension is changed Jul 19, 2022
@lrennels lrennels requested a review from davidanthoff July 19, 2022 19:16
@lrennels lrennels merged commit 5267a01 into master Jul 20, 2022
@lrennels lrennels deleted the dimensions branch July 20, 2022 20:17
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.

1 participant