omerwe / polyfun

PolyFun (POLYgenic FUNctionally-informed fine-mapping)
MIT License
85 stars 21 forks source link

Remove arg `normalize` from `Lasso()` (no longer exists) #183

Closed jdblischak closed 5 months ago

jdblischak commented 5 months ago

Closes #182. Thanks to @Y-Isaac for reporting the error and identifying the fix

The argument normalize was marked as deprecated in scikit-learn 1.0.0 and supposedly removed in 1.2 (I didn't verify exactly when it was removed). See https://github.com/omerwe/polyfun/issues/182#issuecomment-1973820637 for more details

@omerwe would you be able to add a test case of polyfun.py --compute-h2-bins without --nnls-exact?

omerwe commented 5 months ago

@jdblischak thanks as always!

It's been a few years, but I think the reason I didn't use Lasso in the test cases is because it was slightly non-deterministic, even when fixing the random state (small rounding errors). This might have been related to different versions of sklearn or something else, I'm not sure.

In any case, thanks for the pull request. The normalize flag in Lasso by definition didn't do anything in PolyFun, since Lasso receives a precomputed Gram matrix. I'm not sure why I included it in the first place, but it's definitely not needed.