Skip to content

Conversation

davidanthoff
Copy link
Collaborator

Fixes #753.

Before we merge this I'm going to test this with one of our paper replication codes. Also needs a test.

@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #759 into master will increase coverage by 0.01%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #759      +/-   ##
==========================================
+ Coverage   77.82%   77.84%   +0.01%     
==========================================
  Files          36       36              
  Lines        2994     3001       +7     
==========================================
+ Hits         2330     2336       +6     
- Misses        664      665       +1     
Flag Coverage Δ
#unittests 77.84% <85.71%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/core/connections.jl 85.40% <85.71%> (+<0.01%) ⬆️

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 d2ff908...a33a4f0. Read the comment docs.

@lrennels
Copy link
Collaborator

sorry It think I'm a tad behind, but what is the main change we have here then? update_param! no longer dirties a model? Did we change our opinion on what the expected behavior is or change the behavior itself?

@lrennels
Copy link
Collaborator

OH wait sorry ok so we have a new method for update_param! that acts on a model instance instead of a model?

lrennels
lrennels previously approved these changes Sep 11, 2020
@lrennels
Copy link
Collaborator

@davidanthoff I added a unit test, let me know if this looks right for testing?

corakingdon
corakingdon previously approved these changes Sep 18, 2020
Copy link
Collaborator

@corakingdon corakingdon left a comment

Choose a reason for hiding this comment

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

approved! Are we still waiting on @davidanthoff to confirm it works in his case though before merging?

@lrennels
Copy link
Collaborator

I’m just going to update these tests to reflect the desired use case and then we can merge

@lrennels
Copy link
Collaborator

@davidanthoff can we merge this in or would you like to try it out on a model first?

@davidanthoff davidanthoff marked this pull request as ready for review September 24, 2020 00:54
@davidanthoff davidanthoff merged commit 8ae5329 into master Sep 24, 2020
@davidanthoff davidanthoff deleted the update_param_for_mi branch September 24, 2020 00:54
@davidanthoff
Copy link
Collaborator Author

Thanks for the tests!

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

Successfully merging this pull request may close these issues.

update_param! dirties the model definition
3 participants