Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request enhances the propensity score weighting functions by adding method dispatch support for data frames and GLM objects, improving helper function consistency, and expanding test coverage. It enables users to work directly with fitted models and data frames containing propensity scores, making the API more flexible.
- Adds data frame and GLM method dispatch for all weight functions (wt_ate, wt_att, wt_atu, wt_atm, wt_ato, wt_entropy)
- Replaces direct attribute access with helper functions (is_ps_trimmed, is_ps_truncated) for better encapsulation
- Restricts continuous exposure support to only ATE weights and removes continuous option from other estimands
- Significantly expands test coverage with edge cases, integration tests, and cross-package comparisons
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/testthat/test-weights.R | Replaces attribute access with helper functions and adds extensive tests for new data frame/GLM methods, edge cases, and package comparisons |
| man/wt_ate.Rd | Updates documentation to reflect new method signatures, adds examples for data frame and GLM usage |
| R/weights.R | Implements data frame and GLM methods for all weight functions, adds helper functions for propensity score extraction |
| R/weights-utils.R | Adds length validation function and updates exposure type matching to accept restricted valid types |
| NAMESPACE | Exports new S3 methods for data frame, GLM, and default dispatch |
| DESCRIPTION | Moves tidyselect to Imports and adds new suggested packages for enhanced functionality |
Comments suppressed due to low confidence (2)
tests/testthat/test-weights.R:600
- The test removes the 'exposure_type' parameter but the error message being tested might not be relevant to the actual change. The test should verify that auto-detection fails appropriately for continuous data when continuous is not supported.
expect_error(
tests/testthat/test-weights.R:1189
- This test expects an error from wt_ate with an lm object, but lm objects don't inherit from glm, so this should use a different error class or test scenario that actually represents the intended behavior.
expect_error(
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.