Skip to content
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

[WIP] Improve wrapped sklearn class repr - simplify cross val for Dataset / MLDataset #228

Open
wants to merge 7 commits into
base: cv-xarray-issue-204
Choose a base branch
from

Conversation

PeterDSteinberg
Copy link
Contributor

  • Creates an elm.pipeline.steps subpackage for wrapped sklearn transformers / estimators. This will help the repr / str and other __ private methods for the wrapped classes.
  • Moves some of the logic of dask-searchcv PR 61 to Elm


inds = self.splits[n][0] if is_train else self.splits[n][1]

post_splits = getattr(self, '_post_splits', None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TomAugspurger This is a work in progress still - moving the post_splits related CV/sampler logic from dask/dask-searchcv#61 to this new Elm PR that has Elm #221 as a base branch

@PeterDSteinberg
Copy link
Contributor Author

@gbrener - In this PR, there are new modules in elm.pipeline.steps that map to sklearn subpackages, e.g an elm.pipeline.steps.linear_model module that has a LinearRegression wrapped for MLDataset. Here's an example of AffinityPropagration wrapped. A design question:

  • Previously (e.g. elm PR 192) we had a function that used a base class Wrapped and programmatically created all these wrapped classes rather than the new elm.pipeline.steps modules. And with that approach elm.pipeline.steps.linear_model was a Namespace within the elm.pipeline.steps module (having an attribute LinearRegression). That approach had the problem described here on issue 224 where all the classes had the __name__ Wrapped because Wrapped was a class name used in the dynamic creation of wrapped classes. The good thing about the approach in master now with Wrapped is that it is more succinct, but also it means we would need to programmatically create doc strings for LinearModel as wrapped by elm.pipeline.steps.Wrapped. Most of the file diff in this PR is related to changes in elm.pipeline.steps. Do you think I should undo all the new modules in elm.pipeline.steps and fix the repr of Wrapped ? See its current definition in master here - basically it delgates everything to a class given ._cls which is a class and self is passed into _cls for many methods.

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

Successfully merging this pull request may close these issues.

1 participant