Skip to content

Conversation

@gthopkins
Copy link
Contributor

This pull request addresses issue #158 by checking that the sample sizes of the full and reduced models included in rigr::anova.uRegress() are fit on identical sample sizes (as a surrogate check that the models are fitted on identical data among shared variables). This was a problem when a variable included in the full model contained missing data, for which the default action in rigr::regress() is to subset to complete cases, however this makes the anova test invalid without further consideration.

@gthopkins
Copy link
Contributor Author

@svteichman Hi Sarah! I just made a small change to anova.uRegress(), which checks that the same number of samples are used in the full and reduced model, as described in issue #158. This is particularly an issue when the full model contains missing data in one of the variables that is not included in the reduced model. Due to unrelated check errors, I also had to make some edits to the documentation, old tests, and R-CMD-check.yml workflow files. I am particularly unfamiliar with the .yml file, although ChatGPT was particularly helpful in debugging this error. Please let me know if my changes look good to merge—I would also be happy to run this by Amy if you prefer!

@svteichman
Copy link
Collaborator

Thanks @gthopkins! I'll take a look at this tomorrow, especially at the .yaml files (I've recently had to update these for corncob and radEmu, so I imagine some of the same issues were coming up with these ones, related to versions of things being out of date. This is a good reminder for me to check this out for our other lab packages and update those as well!)

Copy link
Collaborator

@svteichman svteichman left a comment

Choose a reason for hiding this comment

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

Overall this looks great @gthopkins. One quick question, why did you make print.augCoefficients and fitted.uRegress exported functions here? I was just wondering, as I couldn't tell if it directly addresses the PR or if it was needed to deal with errors/warnings/notes in the R-CMD-check

@gthopkins
Copy link
Contributor Author

Thank you, @svteichman! As you suggested, I exported print.augCoefficients and fitted.uRegress due to errors/warnings/notes in the R-CMD-check. If this is non-preferable and these ought to remain internal, I can look into a better solution--just let me know!

@svteichman
Copy link
Collaborator

No worries, I think that's totally fine (in my opinion, not a lot of downsides to exporting additional functions if it helps you in some way), I just wanted to check in. I'll merge!

@svteichman svteichman merged commit 48c72c6 into statdivlab:main Apr 16, 2025
5 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.

2 participants