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

Use update_param! instead of set_param! #50

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Use update_param! instead of set_param! #50

wants to merge 7 commits into from

Conversation

lrennels
Copy link
Collaborator

@lrennels lrennels commented Dec 30, 2021

This PR could break existing code that depends on all parameters being shared, as it does to the MimiIWG code, and I can't comprehensively test that so it may be wise to tag a new major version after this change. The other option would be to leave the behavior that all parameters are shared model parameters, but this is not consistent with the desired messaging around how to use update_param!.

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Patch coverage: 98.63% and project coverage change: +0.58 🎉

Comparison is base (a6624f6) 89.41% compared to head (e332c31) 90.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #50      +/-   ##
==========================================
+ Coverage   89.41%   90.00%   +0.58%     
==========================================
  Files          12       12              
  Lines         189      200      +11     
==========================================
+ Hits          169      180      +11     
  Misses         20       20              
Flag Coverage Δ
unittests 90.00% <98.63%> (+0.58%) ⬆️

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

Impacted Files Coverage Δ
src/marginaldamage.jl 69.23% <50.00%> (ø)
src/MimiDICE2010.jl 100.00% <100.00%> (ø)
src/parameters.jl 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lrennels
Copy link
Collaborator Author

lrennels commented Feb 6, 2023

@davidanthoff would you mind merging this and tagging a new breaking version?

@lrennels
Copy link
Collaborator Author

@davidanthoff can you merge this?

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