Skip to content
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

support length 1 arrays and relative metrics #11

Merged
merged 5 commits into from
Jan 9, 2025
Merged

Conversation

elray1
Copy link
Contributor

@elray1 elray1 commented Jan 8, 2025

Two distinct sets of updates here:

  • update the schema to support length 1 arrays. I'm not sure if this is a core thing about YAML or just some behavior in the R packages we're using to read and validate the data structures -- but it seems like to get validations to pass in the case of arrays of strings with length 1, we need to say that valid entries may be either strings or arrays of strings. Added this to the schema and some test cases.
  • support computation of relative metrics:
    • update schema to accept relative_metrics and baseline as properties of target entries
    • add appropriate validations of those things and tests for them
    • if relative metrics are specified, compute them, and add tests for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bulk of this code was previously in test-generate_eval_data.R. It's been moved to this separate helper-*.R file, with some minor updates around the include_rel parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Per my suggestion in hubverse-org/hubEvals#69 (review), I would recommend saving these outputs as test fixtures instead of generating them when the tests are run. From the looks of it, you only need to generate the tables for the relative metrics and subset them to the columns without relative metrics.

Copy link
Contributor Author

@elray1 elray1 Jan 8, 2025

Choose a reason for hiding this comment

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

unfortunately the git diff for this file is useless, diff got confused. For this test file, there are three changes:

  1. deleted check_exp_scores_for_window function (this moved to separate R script)
  2. Added ", no relative metrics" to the description of the first test case, no changes to the test itself.
  3. Added a second test case with relative metrics. This is a near-duplicate of the first test case, but it refers to a config file that includes relative metrics and asks check_exp_scores_for_window to include relative metric results.

@zkamvar zkamvar self-requested a review January 8, 2025 15:45
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

This looks okay to me with the exception of the test helper, as noted in my comment below.

Regarding this comment:

update the schema to support length 1 arrays. I'm not sure if this is a core thing about YAML or just some behavior in the R packages we're using to read and validate the data structures -- but it seems like to get validations to pass in the case of arrays of strings with length 1, we need to say that valid entries may be either strings or arrays of strings. Added this to the schema and some test cases.

This is partially a symptom of jsonlite and partially a symptom of the yaml package. Both have their weird idiosyncrasies and something always gets lost in translation. I think your solution is a good workaround.

Copy link
Member

Choose a reason for hiding this comment

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

Per my suggestion in hubverse-org/hubEvals#69 (review), I would recommend saving these outputs as test fixtures instead of generating them when the tests are run. From the looks of it, you only need to generate the tables for the relative metrics and subset them to the columns without relative metrics.

@elray1 elray1 requested a review from zkamvar January 8, 2025 22:07
zkamvar
zkamvar previously approved these changes Jan 8, 2025
Copy link
Member

@zkamvar zkamvar left a comment

Choose a reason for hiding this comment

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

Thank you! I have one minor suggestion to remove calls to library(), but otherwise this looks good!

tests/testthat/testdata/create_exp_score_fixtures.R Outdated Show resolved Hide resolved
@elray1
Copy link
Contributor Author

elray1 commented Jan 9, 2025

I tried removing the library() calls, but just using rlang::.data didn't work immediately. My take is that this is good enough at this point and it's likely not worth our time to sort out removing that last library(rlang) call.

@zkamvar
Copy link
Member

zkamvar commented Jan 9, 2025

I tried removing the library() calls, but just using rlang::.data didn't work immediately. My take is that this is good enough at this point and it's likely not worth our time to sort out removing that last library(rlang) call.

Yeah, that's a leaky abstraction and definitely not worth it. I'm a little surprised that .data is coming from rlang, since it's a dplyr idiom.

@elray1 elray1 merged commit a7cff83 into main Jan 9, 2025
7 of 8 checks passed
@elray1 elray1 deleted the elr/rel_metrics branch January 9, 2025 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants