Skip to content

Conversation

@seabbs
Copy link
Contributor

@seabbs seabbs commented Jul 18, 2023

This PR closes #272 by implementing an across argument to summarise_scores().

It also:

  • Updates the release version by one minor release (in line with other recent PRs)
  • Adds a news item for this change
  • Adds an example to the summarise_scores() documentation for this new argument.
  • Adds tests that check the equivalence of this approach as well as checking it errors when expected.

Note that because of the multiple duplicate variables in the example data across isn't actually as useful as you might imagine (i.e. because target_end_date and horizon define the same thing).

@seabbs seabbs changed the title Seabbs/issue272 Issue 272: Add an across argument to summarise_scores() Jul 18, 2023
@seabbs seabbs requested a review from nikosbosse July 18, 2023 12:22
@epiforecasts epiforecasts deleted a comment from codecov bot Jul 18, 2023
@epiforecasts epiforecasts deleted a comment from codecov bot Jul 18, 2023
@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #302 (41602c6) into main (3bf41d8) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 41602c6 differs from pull request most recent head 0f9150b. Consider uploading reports for the commit 0f9150b to get more accurate results

@@            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              
Impacted Files Coverage Δ
R/summarise_scores.R 83.33% <100.00%> (+1.93%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@seabbs seabbs added the enhancement New feature or request label Jul 18, 2023
Copy link
Collaborator

@nikosbosse nikosbosse left a 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.

@seabbs seabbs requested a review from nikosbosse July 19, 2023 10:18
@seabbs
Copy link
Contributor Author

seabbs commented Jul 19, 2023

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.

@nikosbosse
Copy link
Collaborator

Nice, looks good! There is just one word or something in the documentation missing, otherwise happy to merge

@nikosbosse nikosbosse self-requested a review July 19, 2023 12:24
@seabbs
Copy link
Contributor Author

seabbs commented Jul 19, 2023

Reworded. Give it a review and will merge when okay'd

@seabbs seabbs merged commit 0a6e98d into main Jul 19, 2023
@seabbs seabbs deleted the seabbs/issue272 branch July 19, 2023 13:03
@nikosbosse
Copy link
Collaborator

Not sure how strongly you feel about linter complaints :D
Happy to merge from my side :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add functionality to "summarise across" columns

2 participants