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

shuffle code around in preparation to add support for actually computing scores #8

Merged
merged 4 commits into from
Dec 24, 2024

Conversation

elray1
Copy link
Contributor

@elray1 elray1 commented Dec 23, 2024

  • move stuff related to getting the output types to use for each metric into utils-metrics.R and add unit tests
  • as part of that, move some small pieces of code related to getting output types and whether or not a target is ordinal into utils-hub_tasks_config.R

@@ -120,34 +120,26 @@ validate_config_targets <- function(webevals_config, task_groups, task_id_names)
}

# check that metrics are valid for the available output types
output_types_for_target <- purrr::map(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved computation of output_types_for_target and target_is_ordinal into their own functions in utils-hub_tasks_config.R. These functions are now called from within get_metric_name_to_output_type.

Slightly awkwardly, to support issuing informative errors, we also compute them below if we need to issue the error.

@@ -251,61 +243,6 @@ validate_config_task_id_text <- function(webevals_config, task_groups, task_id_n
}


#' Get a data frame with 1 row for each metric, matching the metric with the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_metric_name_to_output_type and get_standard_metrics moved to utils-metrics.R. I'm going to need these functions in the code to compute scores, so it's better to have them in a utils file.

zkamvar
zkamvar previously approved these changes Dec 23, 2024
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.

LGTM! I had one question regarding the method for checking for ordinal target types and some stylistic suggestions WRT names, but both can be ignored.

R/utils-hub_tasks_config.R Outdated Show resolved Hide resolved
R/utils-metrics.R Show resolved Hide resolved
tests/testthat/test-utils-hub_tasks_config.R Outdated Show resolved Hide resolved
tests/testthat/test-utils-hub_tasks_config.R Outdated Show resolved Hide resolved
tests/testthat/test-utils-hub_tasks_config.R Outdated Show resolved Hide resolved
tests/testthat/test-utils-metrics.R Show resolved Hide resolved
tests/testthat/test-utils-metrics.R Show resolved Hide resolved
R/utils-metrics.R Outdated Show resolved Hide resolved
R/config.R Outdated Show resolved Hide resolved
R/utils-hub_tasks_config.R Show resolved Hide resolved
get_target_is_ordinal --> is_target_ordinal

Co-authored-by: Zhian N. Kamvar <zkamvar@gmail.com>
@elray1 elray1 requested a review from zkamvar December 24, 2024 15:32
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.

LGTM! Thank you!

@elray1 elray1 merged commit 4ed84f9 into main Dec 24, 2024
7 of 8 checks passed
@elray1 elray1 deleted the elr/refactor_metric_utils branch December 24, 2024 16:52
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