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

New grouped_df class support with soma_adat objects #67

Merged
merged 5 commits into from
Jan 9, 2024

Conversation

stufield
Copy link
Contributor

@stufield stufield commented Jan 4, 2024

Overview of Pull Request

Adding new grouped_df support for the soma_adat class.
The combination of soma_adat, grouped_df was interfering with class reconstitution of attrs.
Needed to make the underlying S3 methods more robust to NextMethod() calls.
No user facing changes (beyond bug-fix), except for the S3 print method for grouped_df soma_adats.

Fixes #66

Main changes

  • cleaned up all dplyr S3 method verbs
  • this involves ensuring rownames and attrs are maintained during verb calls
  • which in turn involves updating the rownames suite of helper functions
  • added grouped_df support (the soma_adat class is maintained)
  • added S3 print method for c("soma_adat", "grouped_df") class
  • new S3 row.names<-.soma_adat() method, that is essentially a
    pass thru to the data.frame method that ensures no inherited class
    dispatch happens (e.g. tbl_df) ... bypassing the rownames warning on tibbles.
  • added new internal helpers: .soma_adat_restore(), .sort_attrs() for the dplyr verb suite
  • minor shoring up of the [ extract method for soma_adat, for the edge-case of grouped_df soma_adats.

Change type

Please check the relevant box(es):

Choose reviewer(s)


Code

This should now work:

test <- dplyr::group_by(example_data, SampleType) 
class(test)
is_intact_attr(test)
dplyr::group_data(test)

foo <- dplyr::filter(test, dplyr::row_number() == 1L)
class(foo)
is_intact_attr(foo)
dplyr::group_data(foo)

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d75438c) 73.64% compared to head (5b0cf9c) 73.88%.

❗ Current head 5b0cf9c differs from pull request most recent head bc418bc. Consider uploading reports for the commit bc418bc to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   73.64%   73.88%   +0.23%     
==========================================
  Files          39       39              
  Lines        1442     1455      +13     
==========================================
+ Hits         1062     1075      +13     
  Misses        380      380              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@stufield stufield force-pushed the add-grouped-df-support branch 3 times, most recently from 499e140 to 63078e6 Compare January 5, 2024 19:57
R/dplyr-verbs.R Outdated Show resolved Hide resolved
R/s3-soma-adat.R Outdated Show resolved Hide resolved
Copy link

@amanda-hi amanda-hi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

- grouped_df data objects were interfering
  with the S3 methods for dplyr verbs
  of the `soma_adat` class once `NextMethod()` was called
- this support now ensures that the group
  methods are maintained, as well as the `soma_adat` class
  itself (with its attributes)
- fixes SomaLogic#66
- now displays Group information
  from a call to the S3 print method
- this method is invoked on `rownmaes()` calls to the
  `soma_adat` class.
- Rather than calling `NextMethod()` which normally
  would invoke `data.frame`, we now force the `data.frame`
  method in case there are `tbl_df` or `grouped_df`
  classes present that would be dispatched.
  Those are bypassed in favor of the `data.frame`
  because `tbl_df` 1) can nuke the attributes, 2)
  triggers a warning about adding rownames to a `tibble`
- now uses internal functions with each other
  as originally intended to avoid redundency
- no need for retro-update-pkgdown.sh
@stufield stufield merged commit c6e2233 into SomaLogic:main Jan 9, 2024
3 checks passed
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.

Error when using dplyr::group_by()
3 participants