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

Make selectors compatible with gtsummary #270

Closed
larmarange opened this issue Aug 22, 2024 · 3 comments · Fixed by #271
Closed

Make selectors compatible with gtsummary #270

larmarange opened this issue Aug 22, 2024 · 3 comments · Fixed by #271

Comments

@larmarange
Copy link
Owner

cf. ddsjoberg/gtsummary#1916

Following evolution of gtsummary, the selectors should be made compatible

@larmarange
Copy link
Owner Author

larmarange commented Aug 22, 2024

Hi @ddsjoberg

could you please have a look at #271?

I have introduced a scope_tidy() function very similar to gtsummary::scope_table_body(). The main difference is that we are taking advantage of the var_class column (if available) to associate the correct class to the tibble generated when data is not provided. I have also prefixed the attributes with "gtsummary." to ensure that they will not be conflict between gtsummary and broom.helpers.

The selectors have been redefined. Just to be noted that there are some duplicates between the two packages: all_categorical, all_continuous, all_contrasts, all_dichotomous, all_interaction, all_intercepts are defined in both packages.

I have made a small change in .select_to_varnames() to rely now on scope_tidy(). As I understood it, you are not using it anymore in gtsummary but rather on cards::process_selectors() https://insightsengineering.github.io/cards/latest-tag/reference/process_selectors.html Am I right? I suppose that these functions are supposed to replace .select_to_varnames() and .formula_list_to_named_list()

I wonder the best approach for broom.helpers to avoid redundancy. Should broom.helpers import these functions from cards (and add a new dependency just for processing selectors)?

Would it be relevant to consider a new package (eg selectors or gtsummary_select or better name) to include all selectors and processors? Would it be worth discussing it?

There is no emergency. Things are working so far. But long-term maintenance could be challenging or source of inconsistencies.

In all cases, it would first require deprecating old functions (.select_to_varnames() and .formula_list_to_named_list()) as they seem to be used in other packages:

@larmarange
Copy link
Owner Author

Good news: reverse dependency checking OK with the PR: https://github.com/larmarange/broom.helpers/actions/runs/10511810718/job/29124978319

@ddsjoberg
Copy link
Collaborator

Hi @ddsjoberg

could you please have a look at #271?

absolutely! I should have communicated more clearly. After you pointed out the oversight in gtsummary, i was planning to create that PR for you.

The selectors have been redefined. Just to be noted that there are some duplicates between the two packages: all_categorical, all_continuous, all_contrasts, all_dichotomous, all_interaction, all_intercepts are defined in both packages.

Gotcha, I think before the v2.0 update, we also had similar overlap where the same functions were defined in both pkgs.

I have made a small change in .select_to_varnames() to rely now on scope_tidy(). As I understood it, you are not using it anymore in gtsummary but rather on cards::process_selectors() https://insightsengineering.github.io/cards/latest-tag/reference/process_selectors.html Am I right? I suppose that these functions are supposed to replace .select_to_varnames() and .formula_list_to_named_list()

You are 100% correct.

I wonder the best approach for broom.helpers to avoid redundancy. Should broom.helpers import these functions from cards (and add a new dependency just for processing selectors)?

If you do import them from cards, cards is designed to be light-weight with few dependencies, so it's not the worst dep to take on.

Would it be relevant to consider a new package (eg selectors or gtsummary_select or better name) to include all selectors and processors? Would it be worth discussing it?
There is no emergency. Things are working so far. But long-term maintenance could be challenging or source of inconsistencies.

Ah, interesting, I agree no emergency for now, but we can discuss at some point. I agree it would make the maintenance easier when thing change (if they change).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants