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

add vignette on group-aware and -wise behavior #453

Merged
merged 5 commits into from
Nov 2, 2023

Conversation

simonpcouch
Copy link
Contributor

Related to #434.

@@ -0,0 +1,172 @@
---
title: "Grouping behavior in yardstick"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally thought maybe this vignette would be called "Group-wise metrics" but it ended up being a bit more general than that.

Copy link
Member

Choose a reason for hiding this comment

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

with a title like that, we might wanna preface a little earlier that this isn't about grouped data sets

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with a title like the one currently suggested or the previous "Group-wise metrics"? and in some ways, this is indeed about grouped data.

Copy link
Member

Choose a reason for hiding this comment

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

i get worried, and confused myself a little bit, when we have this new methods, that share close names with group_by() related stuff.

I was thinking of having a "when we say X we talk about ABC, and when we say Y we talk about XYZ"

* The `name` argument gives the name of the new metric we've created; we'll call ours "accuracy difference."
* The `aggregate` argument is a function defining how to go from `fn` output by group to a single numeric.

The output, `accuracy_diff`, is a function subclass called a `metric_factory`:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Trying to talk about functions as objects while adhering to tidyverse style is a bit difficult here. I'm game for however yall want to approach this. :)

Copy link
Member

@EmilHvitfeldt EmilHvitfeldt left a comment

Choose a reason for hiding this comment

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

Few comments!

vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
@simonpcouch
Copy link
Contributor Author

Much appreciated! Accepted all suggestions.

Copy link
Member

@juliasilge juliasilge left a comment

Choose a reason for hiding this comment

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

Looking fantastic! 🎉

This is such a tiny nit, but I prefer "groupwise" to "group-wise":

vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
vignettes/grouping.Rmd Outdated Show resolved Hide resolved
@simonpcouch
Copy link
Contributor Author

On board for all of your changes, thank you @juliasilge!

Ah, no, that's super important to get right! I'm all for transitioning from "group-wise" to "groupwise". Any thoughts on whether we should then write "group aware" instead of "group-aware"?

@juliasilge
Copy link
Member

Seems like "group-aware" is more common, like this.

@simonpcouch
Copy link
Contributor Author

Awesome, we'll keep that, then.

After putting 9145ba1 together, I do also appreciate that the way we write "groupwise" in text aligns with how it appears in code. :)

Thanks again.🐙

@EmilHvitfeldt EmilHvitfeldt merged commit d9cee7d into main Nov 2, 2023
12 checks passed
@EmilHvitfeldt EmilHvitfeldt deleted the groupwise-vignette branch November 2, 2023 17:34
Copy link

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 Nov 17, 2023
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.

3 participants