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

Fix bug in metrics selection #112

Merged
merged 6 commits into from
Jul 19, 2021
Merged

Fix bug in metrics selection #112

merged 6 commits into from
Jul 19, 2021

Conversation

nikosbosse
Copy link
Contributor

This PR

  • fixes a bug where not selecting 'interval_score' as a metric when using quantile data resulted in an error
  • fixes an unrelated small bug where for binary scores, the wrong data.frame was used to summarise scores
  • creates a new unit test for the bug fix
  • updates the version number and news file

@nikosbosse nikosbosse requested a review from Bisaloo July 19, 2021 16:19
@codecov
Copy link

codecov bot commented Jul 19, 2021

Codecov Report

Merging #112 (4122ed9) into master (559c98c) will increase coverage by 0.11%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #112      +/-   ##
==========================================
+ Coverage   48.74%   48.85%   +0.11%     
==========================================
  Files          18       18              
  Lines        1356     1357       +1     
==========================================
+ Hits          661      663       +2     
+ Misses        695      694       -1     
Impacted Files Coverage Δ
R/eval_forecasts_quantile.R 94.05% <64.70%> (+0.05%) ⬆️
R/eval_forecasts_binary.R 100.00% <100.00%> (ø)
R/eval_forecasts_continuous_integer.R 69.64% <100.00%> (ø)
R/eval_forecasts.R 66.07% <0.00%> (+0.89%) ⬆️

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 559c98c...4122ed9. Read the comment docs.

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

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

Looks good 👍!

The only point that confuses me a little bit (but it's probably because I'm really unfamiliar with data.table): you seem to modify sometimes by copy (e.g., https://github.com/epiforecasts/scoringutils/pull/112/files#diff-a98d021cad877241c9f935dc0f0c81c3cdbb82f3efd796aaaf2788ac390b1c38R62) and sometimes in place (e.g., https://github.com/epiforecasts/scoringutils/pull/112/files#diff-a98d021cad877241c9f935dc0f0c81c3cdbb82f3efd796aaaf2788ac390b1c38R187) in the same function. Does it matter? Or it's just a stylistic preference and it's not important?

@nikosbosse
Copy link
Contributor Author

Hm for some reason when I click on the links they don't bring me to the lines you are referring to.
I think there definitely might be some stylistic inconsistencies in there. So usually data.table is all call-by-reference. Sometimes this can lead to a function modifying the data that you put in the function, which is of course not good. Actually, data.table should copy your input data when you pass it to the function, but that doesn't happen always.
So my idea was to copy input data once (or call as.data.table()) to be sure I'm not modifying any inputs from the user accidentally. Everything that is internal and doesn't need copying should probably be done by reference for increased speed

@nikosbosse
Copy link
Contributor Author

Probably a good idea to make it more consistent in the future

@nikosbosse nikosbosse merged commit 0c8c68e into master Jul 19, 2021
@nikosbosse nikosbosse deleted the bug-fix-metrics branch July 19, 2021 18:00
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.

None yet

2 participants