Skip to content

fixes cross-validation error when wrapping OneHotEncoder - #375

Merged
solegalli merged 6 commits into
feature-engine:mainfrom
noahjgreen295:issue_368
Feb 18, 2022
Merged

fixes cross-validation error when wrapping OneHotEncoder#375
solegalli merged 6 commits into
feature-engine:mainfrom
noahjgreen295:issue_368

Conversation

@noahjgreen295

Copy link
Copy Markdown
Contributor

Fix and unit test for issue 368

@solegalli

Copy link
Copy Markdown
Collaborator

Hi @noahjgreen295

The changes in the wrappers.py file and the additional test with the cv look great! Thank you so much for this.

We do need to remove all the other files from the PR though. The are not relevant for this PR.

Would you be able to do that?

To remove the additional files you need to do:

git checkout main -- path/to/the/file.py

If the tests fail, let me know. But they shouldn't.

@solegalli solegalli changed the title Fix and unit test for issue 368 fixes cross-validation error when wrapping OneHotEncoder Feb 18, 2022
@noahjgreen295

Copy link
Copy Markdown
Contributor Author

0d2fabf

Hi @noahjgreen295

The changes in the wrappers.py file and the additional test with the cv look great! Thank you so much for this.

We do need to remove all the other files from the PR though. The are not relevant for this PR.

Would you be able to do that?

To remove the additional files you need to do:

git checkout main -- path/to/the/file.py

If the tests fail, let me know. But they shouldn't.

Hi Soledad,
Thanks - glad to help!
For the other files to remove, do you mean the ones that tox changed in this commit? If so, I can take care of that right away. Let me know - thanks!

@solegalli

Copy link
Copy Markdown
Collaborator

Yes. Those files :)

@noahjgreen295

noahjgreen295 commented Feb 18, 2022

Copy link
Copy Markdown
Contributor Author

Yes. Those files :)

OK done in latest commit! Unit tests all passed.

@solegalli

Copy link
Copy Markdown
Collaborator

Thank you!

To pass the test we need to shorten this line of code:

tests/test_wrappers/test_sklearn_wrapper.py:333:89: E501 line too long (116 > 88 characters)

would you be able to do that?

if you do black tests/test_wrappers/test_sklearn_wrapper.py it should fix it. Otherwise, you can break it manually.

@noahjgreen295

Copy link
Copy Markdown
Contributor Author

OK done in latest commit. It's strange that my CLI black didn't pick it up (I'm using v. 22.1.0.) That often happens with me at work with that tool (CI black has the issue, CLI black misses it.) Anyway, all committed!

@solegalli

Copy link
Copy Markdown
Collaborator

Thank you so much @noahjgreen295 !

Merging and closing as we speak :)

@solegalli
solegalli merged commit 3a0d0c0 into feature-engine:main Feb 18, 2022
@noahjgreen295

Copy link
Copy Markdown
Contributor Author

My pleasure, glad to help! And thank you for guiding me through the process. Happy to be involved with this great project.

solegalli pushed a commit that referenced this pull request Feb 18, 2022
* added unit test for issue #368; test is now failing before fix is made

* fix to issue #368, making index compatible in OneHotEncoder-generated DataFrame so that it concatenates correctly

* isort/black formatting

* additional style fixes after tox run

* undid tox style fixes on files outside of PR scope

* formatting fix
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.

SklearnTransformerWrapper: cross-validation error when wrapping OneHotEncoder

2 participants