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

projpred-related seed fix and docs/NEWS #1502

Merged
merged 6 commits into from May 23, 2023

Conversation

fweber144
Copy link
Contributor

This fixes a minor bug in PRNG usage of get_refmodel.brmsfit(): The PRNG state should be reset upon exit only if the user-supplied seed is not NULL. See the NEWS.md entry from 8db0bbd for details.

In principle, I would also recommend to replace brms_seed = NULL with brms_seed = NA (i.e., changing the default but also replacing is.null(brms_seed) with is.na(brms_seed) within the function—although length 1 would probably have to be asserted for NA—and adapting the code for brms_seed_k accordingly) because NULL has a special meaning for set.seed()'s argument seed, namely to generate completely unreproducible results (which might sometimes be desired by users). However, it seems like within brms, NA has a special meaning for seed arguments, namely that a seed will be set randomly (see brms::brm()). So in that regard, it's probably better to stick with NULL instead of NA (otherwise, all other seed arguments would have to be adapted for the sake of consistency).

In commit 0c5c8a5, a NEWS.md entry for projpred's augmented-data projection is added at an older version (2.17.0). Back then, I didn't add a NEWS.md entry because projpred's CRAN version didn't support the augmented-data projection at that time. Now, users might be searching for "augmented-data projection" across this NEWS.md file to see when support for it was added (similarly to the latent projection, for which a NEWS.md entry was added at brms version 2.19.0). That's why I'm adding the augmented-data projection entry now.

Furthermore, I would still recommend to remove argument newdata of get_refmodel.brmsfit(). The last state of our discussion on this was at #1292 (comment). There, I said I needed to think about whether a reference model where fit and data don't match complies with projpred's understanding of a reference model. I have come to the conclusion that such a reference model does not comply with projpred's understanding of a reference model: In projpred, the reference model is inseparable from its parameter estimates and the data they are based on (so in case of new data, the reference model would have to be refitted). The corresponding rstanarm method (get_refmodel.stanreg()) also doesn't have a newdata argument. So if you are ok with this, then I would push a commit here that deprecates this argument.

@paul-buerkner
Copy link
Owner

paul-buerkner commented May 23, 2023

Thank you! Yes, feel free to remove the newdata argument in get_refmodel(), also within this PR if you like. Can you please shorten the NEWS update to 1 or 2 lines max (or remove the seed stuff from NEWS altoghther; perhaps I would prefer the latter actually)? The lengthy information gives unncessary emphasize on such a minor thing.

@fweber144
Copy link
Contributor Author

Can you please shorten the NEWS update to 1 or 2 lines max (or remove the seed stuff from NEWS altoghther; perhaps I would prefer the latter actually)?

Sure, I've removed it.

feel free to remove the newdata argument in get_refmodel()

Thanks, I have deprecated it now.

@paul-buerkner
Copy link
Owner

Nice, thank you! Merging now.

@paul-buerkner paul-buerkner merged commit 9440d45 into paul-buerkner:master May 23, 2023
4 of 6 checks passed
@fweber144 fweber144 deleted the projpred_fixes_etc branch May 23, 2023 13:53
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.

None yet

2 participants