adding functions to address issues #22 and #25

kimberlynroosa commented 2 months ago

Adding internal functions for transforming quantile and mean/median data to scoringutils compatible format (forecast object).

github-actions[bot] commented 2 months ago

🚀 Deployed on https://66675a2164b124382b8bc593--hubevals-pr-previews.netlify.app

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 31 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (9b64f39) to head (24b02b3).

:exclamation: Current head 24b02b3 differs from pull request most recent head 970e51a

Please upload reports for the commit 970e51a to get more accurate results.

Files	Patch %	Lines
R/transform_quantile_model_output.R	0.00%	17 Missing :warning:
R/transform_point_model_output.R	0.00%	14 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #27 +/- ## ==================================== Coverage 0 0.00% ==================================== Files 0 2 +2 Lines 0 31 +31 ==================================== - Misses 0 31 +31 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

annakrystalli commented 2 months ago

Looking good in general. Would be good to get some tests added though. A quick way to create a test file would be to use usethis::use_test(). Use this when you have the R script you want to add tests for open for editing or override by providing a file name

annakrystalli commented 2 months ago

Have a look at https://r-pkgs.org/testing-basics.html for more info on R testing

nickreich commented 3 weeks ago

Trying to summarize my somewhat haphazard comments (sorry about that):

[ ] add @noRd tag to the functions
[ ] add just a bit more explanation to the transform_point... function to clarify that one and only one of mean/median output-types will be converted
[ ] leaving the dplyr dependency seems ok to me for now, given that we are relying on things like left_join() for which no other simple/good alternative really exists. If it was just filtering/selecting/piping, then I'd say we should try to get away using just base R. But would be curious for others' opinions on this. Maybe we should be stricter about dependencies! That said, I think we could remove the ImportFrom with the pipe, as we probably could just get away with the base R pipe.

All in all, I think this PR looks good and with just a few minor changes (e.g. just the first two above) I'd be ok merging it in. I went through all of the code, although did not run any of it locally on my machine, just read the R code, etc...

-- UPDATE (6/26/2024): we should remove the "add noRd tag" item above since per @annakrystalli 's comment, it is causing issues now.

hubverse-org / hubEvals

adding functions to address issues #22 and #25 #27

Codecov Report