Skip to content

sample-props alternate implementation #20350

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

Closed

Conversation

adrinjalali
Copy link
Member

@adrinjalali adrinjalali commented Jun 24, 2021

This is an alternate to #16079 which is an implementation of SLEP006 Routing sample-aligned meta-data.

The metadata_requests.py file includes most of the implementation. There are some challenges left which we need to figure out how to tackle.

You can see SLEP examples implemented in test_props.py as test_slep_case* functions, and the documentation under metadata_routing.rst

Open Questions

Closes #9566, Closes #15425, Closes #3524, Closes #15425,
Fixes #4497, Fixes #7136, Fixes #13432, Fixes #4632, Fixes #7646 (add test), Fixes #8127 (add test), Fixes #8158, Fixes #7308, Fixes #21134, Fixes #18159, Fixes #20349
Enables #6322,
TODO:
#12052, #8710, #15282, #2630, #8950, #11429, #15282, #18028, #19465, #20167

Note: since this started from where #16079 was, there are still leftovers from that PR, cleaning them up.

Copy link
Member Author

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we generally want to do with estimators which are not meta-estimators per say, but they do have an estimator constructor arg, and forward {method}_params to fit or score. Should they do any kind of validation, or should they just pass everything along? Right now RFE does validation, but transformed regressor doesn't. I'm not really sure which way to go there.

Comment on lines +1972 to +1976
.fit_requests(sample_weight=True)
.score_requests(sample_weight=True)
)
# The old behavior would be sample_weight=False for "score"
# Do we want to "fix" the issue, or keep the old behavior?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to completely ignore sample_weight

@adrinjalali
Copy link
Member Author

The API side seems getting close to where we're happy with ii (@jnothman tell me if I'm wrong). I'm wondering how we can make this easier to move forward. Should we have a branch on the repo, where we start by merging the API/base stuff in a PR first, and then fixing estimators one by one, and then add some common tests?

@glemaitre you showed interest in the last meeting, any help is welcome :)

@adrinjalali
Copy link
Member Author

It'd also be nice if @thomasjpfan could check the API once again.

@jnothman
Copy link
Member

jnothman commented Oct 3, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment