`fit_control` simplification, `prediction_bounds` bug fix, and improved `SL.hal9001`

tlverse / hal9001

🤠 📿 The Highly Adaptive Lasso

https://tlverse.org/hal9001

GNU General Public License v3.0

49 stars 15 forks source link

`fit_control` simplification, `prediction_bounds` bug fix, and improved `SL.hal9001` #100

Closed rachaelvp closed 2 years ago

rachaelvp commented 2 years ago

Fixed bug with prediction_bounds (a fit_hal argument in fit_control list), which would error when it was specified as a numeric vector. Also, added a check to assert this argument is correctly specified.
Simplified fit_control list arguments in fit_hal. Users can still specify additional arguments to cv.glmnet and glmnet in this list.
Defined weights argument in fit_hal, instead of it being an optional argument in fit_control, to facilitate specification and avoid confusion. This increases flexibility with SuperLearner wrapper SL.hal9001; fit_control can now be customized in SL.hal9001.

rachaelvp commented 2 years ago

@nhejazi this is the error I get when I run make site:

Reading 'man/apply_copy_map.Rd'

caught segfault address 0x200000002, cause 'memory not mapped'

Traceback: 1: apply_copy_map(x_basis, copy_map)

nhejazi commented 2 years ago

thanks also for pointing out the issue with make site, i'll look into this separately

rachaelvp commented 2 years ago

Hey @nhejazi thanks so much for your thorough review of this. I just realized that these changes are not urgent; they can be merged into devel. Even though ... is not listed in fit_hal, the documentation for fit_control states that any arguments to glmnet or cv.glmnet can be passed in fit_hal via fit_control list. This is why were successfully able to pass weights in fit_control list with haldensify and sl3.

I noticed that there is no information on version 0.4.3 in the NEWS file and master is on version 0.4.3. Should something be added for version 0.4.3, such as what's in the NEWS for version 0.4.2?

nhejazi commented 2 years ago

thanks for clarifying @rachaelvp, that's very helpful --- the changes to the fit_hal() constructor happened fast enough that i'd lost track, but it's great to realize that it's not a severe bug. it may be worth discussing adding a check to the fit_control arguments (if we don't already) to make sure that the arguments passed in are in the combined set of formals() of glmnet and cv.glmnet.

yes, that's another good catch, we should definitely add information to the NEWS.md file to describe whatever the new changes are for v0.4.3 of the package. also, if there are any other comments to discuss from the review (e.g., ids in the constructor versus a helper function to create folds to pass in to fit_control in the repeated measures case), we can migrate these to issues so that we can get this merged