fix off by one #100
Merged
fix off by one #100
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abhidg
requested review from
andreww,
colinsauze,
tonymugen,
tlestang,
ljcolling,
Llannelongue and
sadielbartholomew
|
This all looks good to me but I think it would be good to turn the error in #99 into a test case - do we actually need a handful of integration tests where we feed in chosen forcasts and test the whole code? |
This fixes issues in the current CATS implementation which led to #99 Firstly, bisect_left() which finds the index of the closest data point past the job end time, was looking at the datetime attribute of the CarbonIntensityPointEstimate object, which by convention only has the start time of the window for which carbon intensity is reported from ciuk_parse_response_data(). This led to an error where if the job end time fell in the last window, bisect_left() would return None as it did not find a starting datetime past the job end time. This commit fixes this by looking at the end of the window. Secondly, this fixes __len__ on WindowedForecast which had a off by one error on the number of windows. This was triggerred when the number of windows was the same size as the data which should be valid, but gave an empty iterable. Thirdly, fixed a IndexError in rbound estimation that was caused by this new method. This happened when the job end time fell on the last data point. Interpolation between this data point and the next one failed, as there was no succeeding data point. Also corrected off-by-one shifts in the list of windows for the trapezoidal rule. Fixes: #99
carbonintensity.org.uk reports carbon intensity for 48h from the current half hour block in 30min windows. Thus if the API is invoked at 18:59 UTC then the API will return with the starting window at 18:30 UTC. This can lead to a situation where a job with duration=2865 min (47h 45min) would place its end time beyond the forecast end by 15min. Thus the maximum duration for the carbonintensity.org.uk provider is changed to 2850min (48h - 30min)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #100 +/- ##
==========================================
+ Coverage 89.19% 89.21% +0.01%
==========================================
Files 14 14
Lines 546 547 +1
==========================================
+ Hits 487 488 +1
Misses 59 59 ☔ View full report in Codecov by Sentry. |
ljcolling
approved these changes
Jun 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes off-by-one errors in the code which led to #99. Detailed info is in commit messages.