search

ryantibs / quantgen

Tools for generalized quantile modeling
https://ryantibs.github.io/quantgen
14 stars 9 forks source link

Fixes to Ryan's package #1

Closed bnaras closed 4 years ago

bnaras commented 4 years ago

Ryan, The fixes here ensure that the package passes more checks than it did before. More importantly, the fixes here enable the delphi team to use it in production. To submit to CRAN, it need more fixes and gurobi should be optional and there's a way to do that using "suggests" rather than "imports". We can chat about it later if needed.

bnaras commented 4 years ago

Ah, the check directory got included by default which accounted for the large number of changes. Now removed.

Re: your questions:

  1. OK.
  2. I do use a different editor and so some spaces might have crept in. The changes can be summarized as follows: fix imports, fix S3 methods for coef, predict and plot---they have to have the right argument signature for coef, predict; e.g. coef(object, ...). My fix was to merely include obj <- object as the first statement in all those coef and predict functions. Also fixed partial name matching in call to Matrix::bandSparse for argument diagonals.
  3. If that is the case, your package can import Rglpk and use "suggests" for gurobi. Then you can use requireNamespace to detect if it is available. If available, use it or drop to Rglpk. (The calls require and library are not advised as they modify search paths.) It is quite simple for me to do 3 if that is what you wish. Users are far more likely to have Rglpk installed than gurobi. Let me know.
ryantibs commented 4 years ago

Sounds good. If you could implement your suggestion RE point number 3, that would be good. Gurobi is preferred over GLPK because it's much faster (as the example notebook shows). One you do this, I can merge the changes.

I also noticed you had occasionally included gurobi::gurobi and Rglpk::Rglpk_solve_LP. Since these are being imported, this isn't needed, right?

bnaras commented 4 years ago

True. The package prefix is not needed, but it is good to keep it for being completely unambiguous, particularly when you have multiple imported packages giving you similarly named functions, e.g. dplyr::filter versus stats::filter. I'll wrap up 3.

bnaras commented 4 years ago

Imported Rglpk but prefer gurobi if available. So package installers will have to install Rglpk for sure as a backup to ensure that quantgen works off the bat.