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

Reduce dependencies: TMB, coda, cubelyr #106

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Reduce dependencies: TMB, coda, cubelyr #106

merged 3 commits into from
Sep 25, 2020

Conversation

vincentarelbundock
Copy link
Contributor

@vincentarelbundock vincentarelbundock commented Sep 25, 2020

This PR does 5 things:

  • Move TMB to Suggests
  • Move coda to Suggests
  • Remove dependency to cubelyr. This was used only once, so I wrote a few lines of code to "flatten" a data cube.
  • Ordered the dependencies in alphabetical order in DESCRIPTION
  • Add a function called assert_dependency which is called before any functions from TMB or coda are called. When the packages are not installed (should never happen in practice if the user estimated the model themselves), assert_dependency stops execution and asks the user to install the relevant package.

In my initial comment #102 I suggested removing nlme and moving from broom to generics. However, this turned out to be trickier than I expected, so I decided to stop here for now.

Let me know what you think.

@bbolker
Copy link
Owner

bbolker commented Sep 25, 2020

I'm sorry not to have commented on this quicker, I meant to (but I have actually been feeling a little unwell/feverish - wondering if it can possibly be COVID-19, I have very few close contacts ...)

It makes sense to me to move TMB to Suggests, but not coda: the distinction is that TMB is only needed for TMB models (and indirectly for glmmTMB models, although that's handled by glmmTMB), whereas coda is a generic tool for post-processing MCMC output. Also, coda has no dependencies of its own other than lattice (a recommended package), so it adds very little overhead.

I absolutely agree with streamlining, but there is also some small downside to cluttering the code with assert_dependency() statements everywhere, so this is one place where I think it's not worth it.

I might have said the same thing about nlme (in addition to its nlme-specific uses, it is also the source of the VarCorr, fixef, ranef, which are generics that are used by almost all downstream packages. If these were in generics it would make sense to import them from there instead, but I don't think they are (I searched the GH repo - I might have missed them since GH search is so terrible ...)

On the other hand, all of your other suggestions/changes (get rid of cubelyr, eventually replace broom with generics ...) seem very sensible.

Please either (1) convince me that coda should be moved to Suggests:; (2) back that particular change out of your PR; or (3) tell me that you agree, but leave it to me to do the backing-out (either in this branch, or after the PR is merged)

@vincentarelbundock
Copy link
Contributor Author

The main reason to kick coda out would be for someone who uses lme4 and has no use for mcmc. Since that's likely on a non-trivial part of the user base, I thought it justified moving it Suggests.

But it's not a big deal; I'm happy to do whatever you feel more confortable with. This last commit brings coda back in by deleting 6 assert_dependency calls.

The issue with broom->generics is that broom.mixed uses augment_columns a few times. I haven't spent enough time with the internals to figure out if there's an easy substitute.

@bbolker
Copy link
Owner

bbolker commented Sep 25, 2020

I'm not sure I had a really good reason for using augment_columns except that it was there in broom. We could just copy it, although that also feels like bad practice. broom is slightly heavier in terms of requirements than the other packages you're trying to trim (because it makes unrestricted use of tidyverse machinery, so it needs backports, glue, ellipsis, rlang (in addition to generics, which would be a one-for-one swap for broom itself ...)

@bbolker bbolker merged commit 03de1c6 into bbolker:master Sep 25, 2020
@bbolker
Copy link
Owner

bbolker commented Sep 25, 2020

PS let me know how you want your name added to the 'ctb' list in DESCRIPTION (real name, Github name? ORCID?) - or submit another PR for that ...

@vincentarelbundock
Copy link
Contributor Author

Oh, this is what I use usually, but honestly, I don't think it's necessary for such a small thing.

Authors@R: person("Vincent", "Arel-Bundock", 
                  email = "[email protected]", 
                  role = c("aut", "cre"),
                  comment = c(ORCID = "0000-0003-2042-7063"))

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