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.

Sign up for GitHub

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

Jump to bottom

Consistency of arguments across packages, in particular by, group and group_by. #404

Closed
9 of 10 tasks
strengejacke opened this issue May 15, 2024 · 12 comments
Closed
9 of 10 tasks

Consistency of arguments across packages, in particular by, group and group_by. #404

strengejacke opened this issue May 15, 2024 · 12 comments
Labels
Code Style 👩‍💻 Core Packages 📦 Discussion and planning about core packages of easystats Corporate Design / PR 💅

Comments

@strengejacke
Copy link
Member

strengejacke commented May 15, 2024 •
edited

We have

  • insight::export_table(group_by)
  • datawizard::data_partition(group)
  • datawizard::data_tabulate(by)
  • datawizard::means_by_group(group)
  • report::report_sample(group_by)

to name a few. I think we should just use one of the three verbs to indicate grouping, to be more consistent across our easystats packages. What would you suggest?
@strengejacke strengejacke added Core Packages 📦 Discussion and planning about core packages of easystats Corporate Design / PR 💅 Code Style 👩‍💻 labels May 15, 2024
@DominiqueMakowski
Copy link
Member

And we sometimes have at as well...

I think I like by, it's concise + it's also in-line with the recent tidyverse evolution .by

@bwiernik
Copy link
Contributor

And we sometimes have at as well...

I think I like by, it's concise + it's also in-line with the recent tidyverse evolution .by

And with marginaleffects and aggregate()

strengejacke added a commit to easystats/insight that referenced this issue May 16, 2024
@strengejacke
https://github.com/easystats/easystats/issues/404
ccfdf71
strengejacke added a commit to easystats/performance that referenced this issue May 16, 2024
@strengejacke
https://github.com/easystats/easystats/issues/404
24959dd
strengejacke added a commit to easystats/datawizard that referenced this issue May 16, 2024
@strengejacke
https://github.com/easystats/easystats/issues/404
ea9486e
This was referenced May 16, 2024
Merged
Merged
Merged
Merged
@strengejacke
Copy link
Member Author

In parameters::model_parameters(), should we also rename keep and drop into select and exclude? These argument do not select variables, but regression coefficients, but their intention is similar: to select a subset of data. WDYT?

@DominiqueMakowski
Copy link
Member

I'm not sure, I have the feeling that there should be distinct arguments for columns and for rows

@strengejacke
Copy link
Member Author

ok, makes sense. Better for me, so I don't need to touch this ;-)

strengejacke added a commit to easystats/insight that referenced this issue May 16, 2024
@strengejacke
Rename group/group_by arguments into by (#872)
849db1e 
* easystats/easystats#404

* fix data_grid

* fix split_by

* fix

* Update test-marginaleffects.R

* fix

* lintr
etiennebacher added a commit to easystats/datawizard that referenced this issue May 18, 2024
@strengejacke @etiennebacher
Rename group/group_by arguments into by (#502)
d9a40dc 
* easystats/easystats#404

* update snapshots

* lintr, comments

* fix demean()

* fix means_by_group

* fix

* update news

* fix rescale_weights

* silence tests

* Update NEWS.md

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>

* deprecation warnings

* use insight remotes

* Update NEWS.md

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>

* Also address #265

* update docs

* update docs and tests

* Update extract_column_names.R

* update readme

* trigger CI

* revert commits related to aliases

* version

* news

* other remnants

* fix

* do not use devel pkgdown

* lintr

* same

---------

Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@strengejacke
Copy link
Member Author

What about bayestestR::estimate_density()? There's also an at argument.

@bwiernik
Copy link
Contributor

bwiernik commented May 19, 2024 •
edited

What about bayestestR::estimate_density()? There's also an at argument.

Probably worth changing this one. Oddly enough, looks like group_by is already there and currently deprecated?

@DominiqueMakowski
Copy link
Member

Probably worth changing this one

yup

looks like group_by

We had group_by first, then deprecated in favour of at, and now back to by 😅

This was referenced May 20, 2024
Merged
Open
strengejacke added a commit to easystats/modelbased that referenced this issue May 22, 2024
@strengejacke
Rename at arguments into by
ce16e7b 
easystats/easystats#404
@strengejacke strengejacke mentioned this issue May 22, 2024
Merged
@strengejacke
Copy link
Member Author

@DominiqueMakowski and then for modelbased, we rename all ats into bys, too, right?

@DominiqueMakowski
Copy link
Member

Yes

@strengejacke
Copy link
Member Author

What about visualization_matrix()? 🙈

@strengejacke
Copy link
Member Author

Ah, that's only in modelbased. I confused it with visualization_recipe().

This was referenced May 26, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Code Style 👩‍💻 Core Packages 📦 Discussion and planning about core packages of easystats Corporate Design / PR 💅
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bwiernik @DominiqueMakowski @strengejacke