-
Notifications
You must be signed in to change notification settings - Fork 25
Issue 272: Add an across argument to summarise_scores() #302
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #302 +/- ##
==========================================
+ Coverage 89.61% 89.68% +0.07%
==========================================
Files 22 22
Lines 1367 1377 +10
==========================================
+ Hits 1225 1235 +10
Misses 142 142
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
nikosbosse
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks good to me. Only minor point is that we might want to add a warning if there are column names in across that aren't in the data.
|
I extended the do any of the across variables check to cover are any not in the forecast unit. I also added some verbosity to the documentation. |
|
Nice, looks good! There is just one word or something in the documentation missing, otherwise happy to merge |
|
Reworded. Give it a review and will merge when okay'd |
|
Not sure how strongly you feel about linter complaints :D |
This PR closes #272 by implementing an
acrossargument tosummarise_scores().It also:
summarise_scores()documentation for this new argument.Note that because of the multiple duplicate variables in the example data
acrossisn't actually as useful as you might imagine (i.e. becausetarget_end_dateandhorizondefine the same thing).