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

Perhaps put model package in suggests instead of imports? #183

Open
jordansread opened this issue Aug 23, 2020 · 10 comments
Open

Perhaps put model package in suggests instead of imports? #183

jordansread opened this issue Aug 23, 2020 · 10 comments

Comments

@jordansread
Copy link
Collaborator

I think it is excellent to support mac/nix/win right now, but worry about a couple scenarios where it may be limiting to put all of the model packages (the current ones and future models) in imports. Doing so requires that users are able to successfully install all models, but what if they only want to use LER for the helpful tools for one or two?

Likewise, our supercomputing at USGS is still not able to install GLM3r because of some odd library issues, so I presently can't use LER there for the other models w/o creating my own fork and changing around the installations.

I realize this is potentially an odd question, but I think it might be worth a little bit of discussion, especially since all of the installs in imports are quite hefty. I'm also assuming that LER might be gaining models in the future, and perhaps some of those models would be impossible to support for all OS's.

@jorritmesman
Copy link
Collaborator

This is a very good suggestion! We need to think as well how to facilitate additions of new model versions, and how we catch errors if people try to run a model that they haven't installed the (right) package for (we have a check_models function, so maybe there). We're working on it

@jordansread
Copy link
Collaborator Author

I’d be happy to help work on the strategy and code to support this kind of thing if the group is interested.

@tadhg-moore
Copy link
Collaborator

I agree, this makes sense and will make it easier for installation for users who just want to use the features from one or two models. We could try to trim out some of the packages in Imports that are not so neccessary e.g. lintr, reshape2/plyr/dplyr

Would the best strategy be to just place them in Suggests or will we need to re-design the code to check for the packages in specific functions?

@JFeldbauer
Copy link
Collaborator

I started to work on this issue and created a branch on my github https://github.com/JFeldbauer/LakeEnsemblR/tree/no_model_dependency which can now be installed without having to install the model libraries. But we still have some dependencies on the glmtools and gotmtools packages in LER and they both require the corresponding model, so not all tools of LER will work in this version.

@jorritmesman
Copy link
Collaborator

With the latest version of gotmtools (5 minutes ago), gotmtools should be independent. However, glmtools still relies on GLMr. We'll have to see if this is a problem (i.e. if glmtools is only used for GLM-things inside LER, or also in more general ways).

@jorritmesman
Copy link
Collaborator

jorritmesman commented Aug 27, 2020

We have made the following changes (still on Johannes' and my personal Github repo's):

  • Moved the imports of GLM3r, GOTMr, etc. to "Suggests", and also worked on some other dependencies
  • gotmtools no longer requires glmtools, and is now independent of model packages
  • at the start of run_ensemble and cali_ensemble (where the model packages are called), an error message is thrown if you try to run a model for which you haven't installed the corresponding package

The only remaining issue is regarding glmtools. glmtools depends on GLMr (not GLM3r, which we use to run GLM). In LER, glmtools is used for the functions get_ice, get_nml_value, get_var, read_nml, set_nml, and write_nml, in relatively many places and not only for GLM-related code (e.g. FLake also uses .nml files, and we use glmtools functions for it).

We had a look at the glmtools code. As far as we can see, GLMr is only used in the examples and in read_nml (in case the user inserts "template", the template in GLMr is used).

So this leaves us two options: 1) either the dependency of glmtools on GLMr needs to be removed, or 2) we need to copy the nml functions from glmtools into LER. @jread-usgs , since you are the main contributor of the glmtools package, what would you prefer? Do you think that moving GLMr to "Suggests" in glmtools is desirable?

@JFeldbauer
Copy link
Collaborator

I would just like to add that the ggplot-overhaul branch of glmtools uses GLM3r, but as far as I can see uses it at the same places and for the same reasons as GLMr is used in the master branch.

@jordansread
Copy link
Collaborator Author

I think for the same reasons I suggested it here that GLMr or GLM3r should also be "suggests" for glmtools. You can still use the package to interact with outputs created from running the model in a different way.

@tadhg-moore
Copy link
Collaborator

I just checked both branches of glmtools and the GLMr and GLM3r are still in Imports instead of Suggests, @jread-usgs would you be able to make these chances in glmtools and then we can close this issue?

@jordansread
Copy link
Collaborator Author

I think so @tadhg-moore I just need to take a closer look to make sure there aren't additional implications to those packages. Probably a little bit more effort than just changing the DESCRIPTION file since I probably need to adjust the imports for the relevant packages.

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

No branches or pull requests

4 participants