Skip to content

Add set forecast unit#293

Merged
nikosbosse merged 18 commits intomainfrom
add_set_forecast_unit
Jul 14, 2023
Merged

Add set forecast unit#293
nikosbosse merged 18 commits intomainfrom
add_set_forecast_unit

Conversation

@nikosbosse
Copy link
Copy Markdown
Collaborator

@nikosbosse nikosbosse commented Jul 12, 2023

Add a helper function to manually set the forecast unit. Addresses #268.

@nikosbosse nikosbosse requested a review from seabbs July 12, 2023 14:06
Copy link
Copy Markdown
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

Overall LGTM to me but a few niggles and automated lining issues. I also imagine you want to talk about this function/option in the docs for score or check_forecasts and potentially provide an example in the news file.

This is based on an issue right? It should be linked to the PR and in the news item for this change.

It would also be nice to hear what is happening with the 5% coverage drop for convenience functions and why we think this is okay.

@seabbs seabbs linked an issue Jul 14, 2023 that may be closed by this pull request
@nikosbosse
Copy link
Copy Markdown
Collaborator Author

I think I addressed all comments so from my side this could be merged. I have a separate PR that finally makes check_forecast() pipeable and updates the documentation.

@seabbs seabbs self-requested a review July 14, 2023 12:07
@epiforecasts epiforecasts deleted a comment from codecov bot Jul 14, 2023
Copy link
Copy Markdown
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

LGTM to me in the main but there are still some outstanding conversations and I think we haven't discussed the reduction in test coverage?

Address as you wish and give me a review ping.

@seabbs seabbs added the enhancement New feature or request label Jul 14, 2023
@nikosbosse
Copy link
Copy Markdown
Collaborator Author

Thanks a lot! Overlooked a hidden comment.
The coverage drop should have been addressed, I had added another test.. Do you still see a drop? Need to recheck then.
Otherwise should be good for another review!

@codecov
Copy link
Copy Markdown

codecov bot commented Jul 14, 2023

Codecov Report

Merging #293 (01aa42f) into main (255721a) will increase coverage by 0.12%.
The diff coverage is 100.00%.

❗ Current head 01aa42f differs from pull request most recent head 627a3a5. Consider uploading reports for the commit 627a3a5 to get more accurate results

@@            Coverage Diff             @@
##             main     #293      +/-   ##
==========================================
+ Coverage   89.48%   89.61%   +0.12%     
==========================================
  Files          22       22              
  Lines        1351     1367      +16     
==========================================
+ Hits         1209     1225      +16     
  Misses        142      142              
Impacted Files Coverage Δ
R/convenience-functions.R 85.71% <100.00%> (+4.13%) ⬆️
R/utils.R 89.83% <100.00%> (+0.94%) ⬆️

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

Copy link
Copy Markdown
Contributor

@seabbs seabbs left a comment

Choose a reason for hiding this comment

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

All looks good to me now. Rerunnning code cov shows you covered the test gap - nice job!

🥳

@nikosbosse
Copy link
Copy Markdown
Collaborator Author

Thanks a lot! :)

@nikosbosse nikosbosse merged commit 7e6d776 into main Jul 14, 2023
@nikosbosse nikosbosse deleted the add_set_forecast_unit branch July 14, 2023 16:22
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.

Allow users to specify forecast unit variables

2 participants