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.

Sign up for GitHub

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

Jump to bottom

Refactor StackingCVRegressor so that users can obtain meta_features #294

Closed
wants to merge 0 commits into from
Closed

Refactor StackingCVRegressor so that users can obtain meta_features #294

wants to merge 0 commits into from

Conversation

takashioya
Copy link

@takashioya takashioya commented Nov 30, 2017 •
edited
Loading

Description

modify fit and add an attribute self.train_meta_features_ for obtaining train meta-features,
and add a method predict_meta_features for obtaining test meta-features.

Related issues or pull requests

#285
#290

Pull Request requirements

  • Added appropriate unit test functions in the ./mlxtend/*/tests directories
  • Ran nosetests ./mlxtend -sv and make sure that all unit tests pass
  • Checked the test coverage by running nosetests ./mlxtend --with-coverage
  • Checked for style issues by running flake8 ./mlxtend
  • Added a note about the modification or contribution to the ./docs/sources/CHANGELOG.md file
  • Modify documentation in the appropriate location under mlxtend/docs/sources/ (optional)
  • Checked that the Travis-CI build passed at https://travis-ci.org/rasbt/mlxtend

@pep8speaks
Copy link

pep8speaks commented Nov 30, 2017 •
edited
Loading

Hello @takashioya! Thanks for updating the PR.

Line 124:42: E231 missing whitespace after ','

Comment last updated on December 01, 2017 at 07:02 Hours UTC

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling fc4f3ef on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling c77edb3 on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@takashioya
Copy link
Author

Sadly, flake8 ./mlxtend still gives many errors in my environment. I think, however, most of them are not related to my commit.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling 2762a64 on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@takashioya takashioya force-pushed the refactor-meta-features branch from 2762a64 to d5b6f73 Compare November 30, 2017 08:28
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling d5b6f73 on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@rasbt
Copy link
Owner

rasbt commented Nov 30, 2017

Sadly, flake8 ./mlxtend still gives many errors in my environment. I think, however, most of them are not related to my commit.

No worries, you can ignore those (and I can fix them another day). Usually, I say that making sure that any new code is "PEP8"-fine suffices :).

Thanks for the PR, I will take a look at it now and provide my comments :)

@rasbt rasbt mentioned this pull request Nov 30, 2017
Closed
rasbt
rasbt requested changes Nov 30, 2017
View reviewed changes
Copy link
Owner

@rasbt rasbt left a comment

Choose a reason for hiding this comment

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

The PR looks great so far. Here are just some minor suggestions

docs/sources/CHANGELOG.md Outdated
@@ -15,6 +15,8 @@ The CHANGELOG for the current development version is available at
##### New Features

- New `max_len` parameter for the frequent itemset generation via the `apriori` function to allow for early stopping. ([#270](https://github.com/rasbt/mlxtend/pull/270))
- New `store_train_meta_features` parameter for `fit` in StackingCVRegressor. if True, train meta-features are stored in `self.train_meta_features_`.
New `pred_meta_features` method for StackingCVRegressor. People can get test meta-features using this method. ([#294](https://github.com/rasbt/mlxtend/pull/294))
Copy link
Owner

Choose a reason for hiding this comment

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

Unless you really don't like to be mentioned, you can add your name or GitHub handle here. E.g.,

New `pred_meta_features` method for StackingCVRegressor. People can get test meta-features using this method. ([#294](https://github.com/rasbt/mlxtend/pull/294) via [takashioya](https://github.com/takashioya))

mlxtend/regressor/stacking_cv_regression.py Outdated
@@ -66,10 +66,14 @@ class StackingCVRegressor(BaseEstimator, RegressorMixin, TransformerMixin):
be shuffled at fitting stage prior to cross-validation. If the `cv`
argument is a specific cross validation technique, this argument is
omitted.
store_train_meta_features : bool (default: False)
If True, meta-features for training data is stored when you `fit`
training data. You can get them by `self.train_meta_features_`.
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest a minor rephrasing to

     store_train_meta_features : bool (default: False)
          If True, the meta-features computed from the training data used for fitting the
          meta-regressor stored in the `self.train_meta_features_` array, which can be
          accessed after calling `fit`.

Could you also describe the shape/layout of the self.train_meta_features_ array?

mlxtend/regressor/stacking_cv_regression.py Outdated
@@ -163,6 +172,14 @@ def predict(self, X):
else:
return self.meta_regr_.predict(meta_features)

def predict_meta_features(self, X):
Copy link
Owner

Choose a reason for hiding this comment

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

Here, it would be helpful to format it as a docstring so that it shows up in the API documentation. E.g., something similar to the fit docstring. I just see that predict, get_params etc. don't have docstrings as well, but I can fix that later separately after this PR.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling 3067f26 on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@takashioya takashioya force-pushed the refactor-meta-features branch from dc0a15b to 61861dc Compare December 1, 2017 07:21
@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 90.916% when pulling 61861dc on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 90.916% when pulling 61861dc on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@takashioya takashioya closed this Dec 1, 2017
@takashioya takashioya force-pushed the refactor-meta-features branch from 61861dc to 154499e Compare December 1, 2017 07:31
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 90.897% when pulling 154499e on takashioya:refactor-meta-features into cca77ac on rasbt:master.

@takashioya takashioya mentioned this pull request Dec 1, 2017
Merged
7 tasks
This was referenced Dec 4, 2017
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rasbt rasbt rasbt requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@takashioya @pep8speaks @coveralls @rasbt