Remove deprecations #713
Remove deprecations #713
Conversation
Codecov Report
@@ Coverage Diff @@
## master #713 +/- ##
==========================================
+ Coverage 80.15% 80.64% +0.48%
==========================================
Files 38 38
Lines 2882 2867 -15
==========================================
+ Hits 2310 2312 +2
+ Misses 572 555 -17
Continue to review full report at Codecov.
|
| Now that we have our post-trial function, we can proceed to obtain our two models and run the simulation. Note that we are using a Mimi MarginalModel `mm` from MimiDICE2010, which is a Mimi object that holds both the base model and the model with the additional pulse of emissions. | ||
|
|
||
| ```jldoctest tutorial4; output = false, filter = r".*"s | ||
| ```julia |
There was a problem hiding this comment.
removed the doctest for now because MimiDICE2010 uses .marginal instead of .modified, can re-add it once the correct version of that model is used
|
NOTE to Lisa change the |
|
Next step here is that @ckingdon95 approves and then we merge, right? |
There was a problem hiding this comment.
@lrennels looks great to me! I pushed a few small typos and fixes, and left a couple questions here
| # Deprecated int indexing should still run | ||
| @test x[3] == time_dim_val[3] | ||
| x[TimestepIndex(3)] = temp_dim_val[3] | ||
| @test x[TimestepIndex(3)] == temp_dim_val[3] | ||
| reset_time_val(x, time_dim_val) | ||
|
|
There was a problem hiding this comment.
should we leave part of this test to test that this now errors? (might help with our coverage of the functions we have deprecated, but the code is still there?)
There was a problem hiding this comment.
I guess this is a broader question about coverage of deprecated functions. @davidanthoff is there a way for us to list certain functions that the coverage tests don't care about? because this PR is definitely going to decrease our testing coverage unfortunately
There was a problem hiding this comment.
Yea, it is going to decrease coverage. The other option I guess is to add tests for errors and then remove them when we remove the specific error messages in a later release I just wasn't sure if that was overkill and wanted to keep testing clean ... but coverage then goes down.
There was a problem hiding this comment.
@test_throws seems the right approach for this kind of situation?
There was a problem hiding this comment.
@davidanthoff I added back in the tests for old behavior that were already there, now with @test_throws. Now overall test coverage for Mimi has increased, but the diff coverage is not 100% because there were some deprecated Int-indexing functions that were not previously being tested, and that this PR changed from warnings to errors. Do you want me to add individual tests for each of these, or should we ignore it?
|
@lrennels I noticed some tests that are failing, I think because of actual changes. I'm going to go through and address them now! |
|
@ckingdon95 I think I just fixed one of them one second I'll push! |
|
This one is good now? @ckingdon95 can you approve? |
This PR addresses #585 and transitions from v0.10.0 to v1.0.0 by removing deprecation warning code so that deprecated syntax will error. Note that a small drop in coverage is inevitable since we are leaving in a few functions but no longer supporting, and thus no longer testing, them.