Skip to content

Enable sklearn 1.2#712

Merged
kbattocchi merged 4 commits intomainfrom
kebatt/updateSklearn
Dec 22, 2022
Merged

Enable sklearn 1.2#712
kbattocchi merged 4 commits intomainfrom
kebatt/updateSklearn

Conversation

@kbattocchi
Copy link
Copy Markdown
Member

Fixes #711

@kbattocchi kbattocchi force-pushed the kebatt/updateSklearn branch 2 times, most recently from f6170f2 to 8814200 Compare December 22, 2022 13:36
@fverac
Copy link
Copy Markdown
Collaborator

fverac commented Dec 22, 2022

Given that we are trying to support multiple sklearn versions with our package, in the future should we consider testing across multiple sklearn versions as well?

Because right now our tests only check if our package works with the latest compatible version of sklearn right?

@fverac
Copy link
Copy Markdown
Collaborator

fverac commented Dec 22, 2022

Should we make a minor update to the cate_treatment_names docstrings which currently say
Available only when the featurizer is not None and has a method: `get_feature_names(feature_names)

Update to something like
Available only when the featurizer is not None and has one of the following methods: `get_feature_names(feature_names) or get_feature_names_out(feature_names)

@fverac
Copy link
Copy Markdown
Collaborator

fverac commented Dec 22, 2022

Should we make a minor update to the cate_treatment_names docstrings which currently say Available only when the featurizer is not None and has a method: `get_feature_names(feature_names)

Update to something like Available only when the featurizer is not None and has one of the following methods: `get_feature_names(feature_names) or get_feature_names_out(feature_names)

Also similar thing in _set_transformed_treatment_names in TreatmentExpansionMixin

@kbattocchi
Copy link
Copy Markdown
Member Author

Yes, systematically testing against a wider range of dependencies has long been something we've wanted to support, but our test matrix is already quite large. This is perhaps something that would be better addressed by nightly tests, which we could add when moving over to GitHub Actions.

But also note that Python 3.7 support was dropped by scikit-learn a while ago, so we're still testing against scikit-learn 1.0.3 when we run our Python 3.7 tests.

@kbattocchi kbattocchi disabled auto-merge December 22, 2022 16:29
@kbattocchi
Copy link
Copy Markdown
Member Author

I'm going to defer the docstring updates to a subsequent PR, because there are some additional changes to our name propagation logic that I think would simplify things and which can all go in together.

@kbattocchi kbattocchi enabled auto-merge (rebase) December 22, 2022 17:11
@kbattocchi kbattocchi merged commit b84072b into main Dec 22, 2022
@kbattocchi kbattocchi deleted the kebatt/updateSklearn branch December 22, 2022 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Should we use get_feature_names_out instead of get_feature_names?

3 participants