rename target_observations to oracle_output #62
Conversation
… primary code, examples, and unit tests
| @@ -122,7 +125,7 @@ score_model_out <- function(model_out_tbl, target_observations, metrics = NULL, | |||
| #' Get scoring metrics | |||
| #' | |||
| #' @param forecast A scoringutils `forecast` object (see | |||
| #' [scoringutils::as_forecast()] for details). | |||
There was a problem hiding this comment.
scoringutils::as_forecast no longer exists; as_forecast_doc_template seems like a weird thing to be referring to, but it seemed to have the most useful/general documentation of the topic.
There was a problem hiding this comment.
Sorry for not getting to this earlier. This looks good, the only thing that I was not expecting was the new make_hubExamples_snapshots "helper" function.
I added a suggestion for the documentation text. Beyond that, There are two concerns I have:
- there are two CSV files (one of which is 23K) that are unused and should be removed.
- the process for generating the test data relies on a snapshot of hubExamples and nearly quadruples the size of the package. This is a problem because it will miss any changes that happen in hubExamples that could affect these tests.
I think number 1 should be taken care of in this PR and I would be happy to tackle a solution to number 2 in another PR.
There was a problem hiding this comment.
We import hubExamples as a suggested package, so why not just use these transformations in the helper functions instead of wrapping the output of the data?
At the moment, the new helper files amount to over a megabyte of storage:
$ ls -larth tests/testthat/helper-*
-rw-r--r-- 1 zkamvar staff 5.3K Sep 4 17:25 tests/testthat/helper-pmf_test_expected_merged_forecast.R
-rw-r--r-- 1 zkamvar staff 5.6K Sep 4 17:25 tests/testthat/helper-pmf_test_model_out_tbl.R
-rw-r--r-- 1 zkamvar staff 267K Dec 2 07:50 tests/testthat/helper-hubex_forecast_oracle_output.R
-rw-r--r-- 1 zkamvar staff 936K Dec 2 07:50 tests/testthat/helper-hubex_forecast_outputs.R
-rw-r--r-- 1 zkamvar staff 951B Dec 2 07:50 tests/testthat/helper-pmf_test_oracle_output.R
-rw-r--r-- 1 zkamvar staff 843B Dec 2 07:50 tests/testthat/helper-pmf_test_target_observations.R
| ) | ||
|
|
||
| exp_forecast <- utils::read.csv( | ||
| test_path("testdata/exp_forecast_hubExamples_2.csv") |
There was a problem hiding this comment.
Should this file (and exp_forecast_hubEsxamples_1.csv) be removed? It's not being used anywhere anymore.
Co-authored-by: Zhian N. Kamvar <[email protected]>
|
Thanks for the review! I dealt with the easy ones. r.e. test data from hubExamples, I've been unclear on recommended practice here. At some point, earlier discussion had led to a consensus that test data should live within the package being tested. But I agree that it would be easier not to manage separate copies of the data! |
all references are changed throughout documentation, primary code, examples, and unit tests. This resolves #59 .