-
Notifications
You must be signed in to change notification settings - Fork 13
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
[ENH]: start refactoring pipelines module #14
Conversation
I am still having some difficulty debugging the |
This is exactly the problem I encountered and why I gave up and finally
indexed with integer indices. I did not wanted to loose time and make
things work.
But i agree it would be much cleaner
Le ven. 23 avr. 2021 à 23:13, Denis A. Engemann ***@***.***>
a écrit :
… I am still having some difficulty debugging the test_pipelines.py
@DavidSabbagh <https://github.com/DavidSabbagh> @agramfort
<https://github.com/agramfort>
I absolutely want things to be indexible by column names, not integer
indices.
The ExpandFeatures class must be probably updated to return a DatafFrame.
The code inside ExpandFeatures is the problem, all passes when not
expanding interaction effects.
As it is in this PR, scikit-learn complains becaus –– I guess –– it cannot
drop the indicator colum based on a name, as ExpanFeagures returns an
array, not a DataFrame.
Will push/investigate more tomorrow.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#14 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIC2QGTJLQNGOBS5MWDQG2LTKHPGRANCNFSM43LF33RA>
.
|
I think we have to make clear design choices here and require that the code here only supports DataFrames all the way down. It will simplify things; working on a fix in the meantime. |
@agramfort @DavidSabbagh there is more to be done (especially tests) but I think this is already something. If we can do the necessary to merge this, I think, we make a step ahead. |
meegpowreg/pipelines.py
Outdated
estimator : scikit-learn Estimator object. | ||
The estimator object. Defaults to None. If None, RidgeCV is used with | ||
generalized cross validation for the regularization parameter alpha. | ||
A logarithmic space between -3 and 5 is visited (100 values). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering if we should not make 3 functions
make_filter_bank_transformer
make_filter_bank_regressor
make_filter_bank_classifier
where make_filter_bank_transformer returns the first steps of the pipeline without the scaler and the RIdgeCV
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking in that direction too. make_filter_bank_regressor/classifier
would then just be high-level convenience functions that work API-wise like the current make_filter_bank_model
?
Co-authored-by: Alexandre Gramfort <[email protected]>
Co-authored-by: Alexandre Gramfort <[email protected]>
Co-authored-by: Alexandre Gramfort <[email protected]>
yes !
… |
@agramfort next milestone unlocked :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
besides LGTM
Co-authored-by: Alexandre Gramfort <[email protected]>
Co-authored-by: Alexandre Gramfort <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dengemann merge if happy
Do not merge.
This is still broken but you can see already a bit where it's going.
Addresses multiple issues.