Time Dimension Work #811
Time Dimension Work #811
Conversation
Codecov Report
@@ Coverage Diff @@
## master #811 +/- ##
==========================================
+ Coverage 83.96% 84.25% +0.29%
==========================================
Files 39 39
Lines 3479 3551 +72
==========================================
+ Hits 2921 2992 +71
- Misses 558 559 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| """ | ||
| function _check_first_last(obj::Union{Model, ModelDef}; first::NothingInt = nothing, last::NothingInt = nothing) | ||
| times = time_labels(obj) | ||
| !isnothing(first) && !(first in times) && error("The first index ($first) must exist within the model's time dimension $times.") |
There was a problem hiding this comment.
I like this way of checking conditions to throw an error, without writing the whole if-then!
src/core/dimensions.jl
Outdated
| new_last = last(new_keys) | ||
|
|
||
| # (1) check that the shift is legal | ||
| if isa(obj, ModelDef) |
There was a problem hiding this comment.
Just a thought: I get that we want different error messages for model vs component, but it feels a little weird to have identical sets of checks. Is there a chance that time shifts on the two will behave differently in the future, or is it too awkward to just format the text depending on model vs component?
There was a problem hiding this comment.
good catch I agree, probably just a relic of thinking they'd need to be more different than they are
src/core/dimensions.jl
Outdated
|
|
||
| else # variable timesteps | ||
| start_idx = 1 # can be assumed since we cannot move the time forward | ||
| new_last < curr_last ? end_idx = findfirst(isequal(curr_last), new_keys) : end_idx = length(curr_keys) |
There was a problem hiding this comment.
Should the first return value instead be "findfirst(isequal(new_last), curr_keys)"? It looks like we're looking for how much of curr_keys ought to be retained in the case where the new values end before the current values. Which would be the portion of it that goes until the new ending.
There was a problem hiding this comment.
nice catch you're right!
#808 and #805