Skip to content

New metric vec template#337

Merged
EmilHvitfeldt merged 72 commits intomainfrom
new-metric_vec_template
Nov 27, 2022
Merged

New metric vec template#337
EmilHvitfeldt merged 72 commits intomainfrom
new-metric_vec_template

Conversation

@EmilHvitfeldt
Copy link
Copy Markdown
Member

@EmilHvitfeldt EmilHvitfeldt commented Oct 26, 2022

This PR adds a refactoring of metric_vec_template() into numeric_metric_vec_template(), class_metric_vec_template() and prob_metric_vec_template().

At the same time it also refactors validate_truth_estimate_checks() into validate_factor_truth_factor_estimate(), validate_factor_truth_metrix_estimate(), validate_numeric_truth_numeric_estimate(), and validate_binary_estimator() which are being called inside *_vec() functions.

Old functions are soft deprecated

@EmilHvitfeldt
Copy link
Copy Markdown
Member Author

@DavisVaughan You are NOT allowed to look at this yet! I need to sleep and do another pass

Copy link
Copy Markdown
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

I think this is definitely moving in the right direction. I think this idea of moving validation into the metric functions themselves is really smart, and very powerful!

I provided some fairly high level feedback. My main thought (which I have put in detail below, mainly here #337 (comment)) is that I'm wondering if we can make this even simpler, more flexible, and more robust by breaking down the jobs performed by the 3 new vec-template functions into multiple helper functions that we'd force the metric functions to call themselves.

I think I was a bit obsessed with trying to shoehorn everything into 1 metric_vec_template() function, but that has proven to be pretty strict and has kept some people from being able to write their extension functions.

Instead, exporting a number of optional check/validate/na_rm functions that authors could use in their own metrics as needed feels much less restrictive.

As I mention below, it also has the massive benefit of not needing to write your *_impl() function inside the *_vec() function (because we needed to use R's scoping rules weirdly), which I think makes this heavily worth considering

We can schedule some time to talk ASAP if you want to discuss this further

Comment thread NAMESPACE
Comment thread NEWS.md Outdated
Comment thread R/prob-classification_cost.R Outdated
Comment thread R/validation.R Outdated
Comment thread NEWS.md
Comment thread R/template.R Outdated
Comment thread R/template.R Outdated
Comment thread R/template.R Outdated
Comment thread R/validation.R Outdated
Comment thread R/validation.R Outdated
EmilHvitfeldt and others added 4 commits November 22, 2022 15:26
Copy link
Copy Markdown
Member

@DavisVaughan DavisVaughan left a comment

Choose a reason for hiding this comment

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

A number of comments about naming, and a request to open a few issues related to supporting class-pred in a better way and some issues with the curve functions, but overall seems like a really nice simplification, so I'm going to approve alongside this feedback

After addressing my comments, I would encourage you to do one big self review for grammar and documentation consistency, as this touches a lot of files and it is easy to get things out of sync

Comment thread NAMESPACE Outdated
Comment thread NAMESPACE Outdated
Comment thread NAMESPACE
Comment thread NAMESPACE Outdated
Comment thread NEWS.md Outdated
Comment thread R/deprecated-template.R
Comment thread R/prob-roc_curve.R Outdated
Comment thread R/validation.R Outdated
Comment thread R/validation.R
Comment thread R/validation.R Outdated
Comment thread NEWS.md Outdated
Comment thread NEWS.md Outdated
@EmilHvitfeldt EmilHvitfeldt merged commit 8d41dc5 into main Nov 27, 2022
@EmilHvitfeldt EmilHvitfeldt deleted the new-metric_vec_template branch November 27, 2022 06:07
@github-actions
Copy link
Copy Markdown

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants