projpred: `Suggests:` instead of `Imports:`?

[fweber144]

Could projpred be moved from Imports: to Suggests:? Currently,

library(brms)
library(projpred)
detach("package:projpred", unload = TRUE)

produces the warning

‘projpred’ namespace cannot be unloaded:
  namespace ‘projpred’ is imported by ‘brms’ so cannot be unloaded

which does not occur for emmeans, for example (which is in Suggests:):

library(brms)
library(emmeans)
detach("package:emmeans", unload = TRUE)

If you want, I can create a PR for that.

[paul-buerkner]

I think emmeans uses some magic so that methods can be registered to the right generics even when a package is in suggests and then loaded when needed. I would also prefer projpred under suggests. so if you can check what emmeans does and make sure that works in projpred as well that would be really nice!

Frank Weber @.***> schrieb am Mi., 1. Sept. 2021, 09:17:

Could projpred be moved from Imports: to Suggests:? Currently,

library(brms)

library(projpred)

detach("package:projpred", unload = TRUE)

produces the warning

‘projpred’ namespace cannot be unloaded:

namespace ‘projpred’ is imported by ‘brms’ so cannot be unloaded

which does not occur for emmeans, for example (which is in Suggests:):

library(brms)

library(emmeans)

detach("package:emmeans", unload = TRUE)

If you want, I can create a PR for that.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/paul-buerkner/brms/issues/1222, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCW2AH243IEJMPRTC5NNPDT7XHOXANCNFSM5DFZNRCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[fweber144]

I experimented a bit with it. Simply moving projpred from Imports: to Suggests: does indeed not remove that warning. Unfortunately, using the emmeans way in projpred would require some work for which I currently don't have the time. I'll create an issue on projpred's issue tracker so this can be considered in the future. But for now, I think I would simply create a PR here for brms which moves projpred from Imports: to Suggests:. Are you ok with that?

[paul-buerkner]

I doubt just moving to suggests will work. you can try it out and then run cmd check. I assume it will complain.

Frank Weber @.***> schrieb am Mi., 1. Sept. 2021, 10:39:

I experimented a bit with it. Simply moving projpred from Imports: to Suggests: does indeed not remove that warning. Unfortunately, using the emmeans way in projpred would require some work for which I currently don't have the time. I'll create an issue on projpred's issue tracker so this can be considered in the future. But for now, I think I would simply create a PR here for brms which moves projpred from Imports: to Suggests:. Are you ok with that?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/paul-buerkner/brms/issues/1222#issuecomment-910068778, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADCW2AB6JGCGJNG6MR4TTWTT7XREDANCNFSM5DFZNRCA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[fweber144]

Yes, you're right. I forgot the imports of projpred functions in the NAMESPACE file. I'll see how to deal with it.

[fweber144]

There seem to be several ways around that, see e.g. here and here. The downside would be a breaking change because users would have to load or attach projpred themselves if they want to use it. But on the other hand, I think that makes sense for a package which is in Suggests. Not only from a conceptual perspective, but also because currently, users might not even notice to be using projpred. In terms of citations for projpred, that would be disadvantageous.

From the two links above, I would prefer this solution which requires R >= 3.6.0, though. And in conjunction with roxygen2, it requires to run library(projpred) before documenting via devtools::document(). But then it works fine, as I could confirm. An alternative would be this solution based on vctrs::s3_register(). That vctrs::s3_register() solution is basically what is already happening with the emmeans package (see brms's .onLoad()) and what is proposed in the second link above (for the connection to emmeans, see this line).

[paul-buerkner]

Thank you! This breaking change is acceptable to me. Can you explain why you prefer this one solution over the other one currently used in emmeans?

[fweber144]

Simply because it seems like the intended (less "hacky") way from R 3.6.0 on. That requirement for library(projpred) before documenting via devtools::document() might be annoying, though. And unsafe, too. For example, for running R CMD check via RStudio's keyboard shortcut, I had to uncheck "R CMD check" under "Automatically roxygenize when running:" in RStudio's options for this project. That could cause outdated documentation during such an R CMD check run, though. Perhaps there's a way to run arbitrary R code before executing the command from an RStudio keyboard shortcut (I'm thinking of "addins"), but I haven't found such a way yet.

[paul-buerkner]

To be honest, I don't want to make my brms build toolchain more complicated just to move projpred to Suggests. So I would prefer the emmeans approach if feasable (or another approach that does not require the described workaround when building the package).

[fweber144]

Sure, I understand that. I'll try that alternative solution.

[paul-buerkner]

Thank you for handling this issue!

[fweber144]

Just FYI: I found a (very) minor downside of the vctrs::s3_register() approach we are using now: In the documentation for get_refmodel.brmsfit(), we don't get ## S3 method for class 'brmsfit' in section "Usage" anymore.

[paul-buerkner]

Thanks! Perhaps thsis may even be added somehow manually via Roxygen tags if we need to but I guess it is just fine without it.