search

tlverse / hal9001

🤠 📿 The Highly Adaptive Lasso
https://tlverse.org/hal9001
GNU General Public License v3.0
49 stars 15 forks source link

Formula change #93

Closed Larsvanderlaan closed 3 years ago

Larsvanderlaan commented 3 years ago

Totally new formula interface with enhanced capabilities. You can now specify penalization factors, smoothness_orders, and the number of knots for each variable for every single term separately using the powerful "h" function. You can also do "h(X) + h(W)" which will generate and concatenate the two basis function terms. Some nice looking into parent environments for tuning parameters is used. Also makes it straight forward in the future to add new formula features by changing the h function.

I no longer support the ., .^2, etc formulas but I will try to add this.

Larsvanderlaan commented 3 years ago

Apparently I messed up the AppVeyor.

jeremyrcoyle commented 3 years ago

Apparently I messed up the AppVeyor.

I know @nhejazi is working on replacing AppVeyor with GitHub Actions. Not sure if he's migrated this package yet.

nhejazi commented 3 years ago

Actions is already enabled for this package, and the checks seem to be passing. @jeremyrcoyle, I think you might need to manually deactivate Appveyor for this package, as was necessary for some of the others.

Larsvanderlaan commented 3 years ago

I would hold off on merging this. I changed smoothness_orders to smoothness_order which may cause dependent code to break. Also my coVPN analysis depends on the current devel version of formula. I think it is probably best to change back to smoothness_orders and I think the new formula interface will then function in the same way as the current one, but I need to check this.

nhejazi commented 3 years ago

@Larsvanderlaan Thanks for these edits, I'm looking forward to trying out the simplified formula interface. I agree that it's best to move back to smoothness_orders for the sake of maintaining some degree of backwards compatibility. Once you've made this change, let's go ahead and merge this into devel, which I'll shortly thereafter merge into master so that we can get the newest version on CRAN.

For the CoVPN repo, yours and my analyses may be the only ones using hal9001, so if the latest devel is what your analysis is based on, we can tag that version in a release and pin the renv library to that version. Let me know how that sounds and I can make both sets of changes.

Larsvanderlaan commented 3 years ago

@nhejazi Hi Nima, I believe this new version will be backwards compatible with the previous formula interface (assuming some of the more nuanced features like "i" and "d" to specify monotonicity were not used). At least for my COVPN analysis, I believe it will run without error. Although to be safe, especially if more changes are made, I think pinning to the latest devel version is a good idea.

nhejazi commented 3 years ago

great, thanks, @Larsvanderlaan -- it looks like the change for smoothness_orders is already set, so are we all set to merge this? just want to confirm. i've just created the covpn tag for the current devel branch (https://github.com/tlverse/hal9001/releases/tag/covpn), and i will set that version in the renv.lock file in the CoVPN repo shortly

Larsvanderlaan commented 3 years ago

Hi @nhejazi, Yes, this is ready to merge. Thank you for your help and the covpn changes.