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

allow for ANY as weight type backup for n_mean_var() #19

Merged
merged 4 commits into from
Feb 3, 2025

Conversation

malcolmbarrett
Copy link
Contributor

This PR allows for n_mean_var() to take a weight of any type as long is it can pass through as.double(). Basically, it adds a c("thing", "ANY") signature for the existing input types. It also treats a conversion warning as an error.

The motivation for this PR is to allow for smd to accept weights that are classed vectors. We use it in our causal inference work and some of our tooling will be expanding in this direction

@bsaul
Copy link
Owner

bsaul commented Jan 13, 2025

@malcolmbarrett - thanks for the PR. Happy to discuss the proposal. I hesitate to add support for dispatching on "ANY" class of weights, as I'm generally averse to weakening what limited form of type-like safety we can get in R (I recognize you're trying to catch ill-defined behavior). I'll think on other ways to get what you're after.

Also, if you want to the w argument to dispatch on "ANY" why not the x argument too?

@malcolmbarrett
Copy link
Contributor Author

malcolmbarrett commented Jan 13, 2025

You can see the type of class I hope to extend this to here r-causal/propensity#10, if it helps give you any ideas. I don't use S4, so maybe there is a better way. But I don't see this being much of an issue in practice because as.double() serves as a pretty good guard (except I wish it would error rather than warn in many cases!)

Anyway, thanks for considering. It would help us a lot to be able to be more flexible here

@malcolmbarrett
Copy link
Contributor Author

Also, if you want to the w argument to dispatch on "ANY" why not the x argument too?

w is better defined in general than x. It needs to be a numeric or castable to a numeric. But I don't have a strong opinion about it since x works well as-is for our needs

@malcolmbarrett
Copy link
Contributor Author

This is a very small perspective into the stability of this change as I think the bigger concern is to direct users, not developers, but reverse dependencies do well with this PR

> revdepcheck::revdep_check()
── INIT ───────────────────────────────────────────── Computing revdeps ──
── INSTALL ───────────────────────────────────────────────── 2 versions ──
Installing CRAN version of smd
Installing DEV version of smd
── CHECK ─────────────────────────────────────────────────── 4 packages ──
✔ cardx 0.2.2                            ── E: 0     | W: 0     | N: 0    
✔ gtsummary 2.0.4                        ── E: 0     | W: 0     | N: 0    
✔ smdi 0.3.1                             ── E: 1     | W: 0     | N: 0    
✔ tidysmd 0.2.0                          ── E: 0     | W: 0     | N: 0    
OK: 4                                                                   
BROKEN: 0
Total time: 14 min

Of course, your main reverse import is from me 😝

@bsaul
Copy link
Owner

bsaul commented Jan 28, 2025

@malcolmbarrett - sorry the delay. This seems good. Can you:

  • bump the minor version in DESCRIPTION
  • optional but encouraged: add yourself as a contributor in DESCRIPTION

Thanks for the contribution!

@malcolmbarrett
Copy link
Contributor Author

Done, thanks!

@bsaul
Copy link
Owner

bsaul commented Feb 3, 2025

Thanks!

@bsaul bsaul merged commit 110f447 into bsaul:master Feb 3, 2025
7 checks passed
@bsaul
Copy link
Owner

bsaul commented Feb 3, 2025

@malcolmbarrett - I'll submit to CRAN in the next few days.

@malcolmbarrett
Copy link
Contributor Author

Much appreciated, @bsaul! No hurry on our end

@bsaul
Copy link
Owner

bsaul commented Feb 12, 2025

FYI - submitted to CRAN. Usually takes a couple of days to show up.

@bsaul
Copy link
Owner

bsaul commented Feb 12, 2025

Dear maintainer,
 
thanks, package smd_0.8.0.tar.gz is on its way to CRAN.
 
Best regards,
CRAN teams' auto-check service
Package check result: OK

No changes to worse in reverse depends.

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.

2 participants