Skip to content

[WIP] improving equality test in sklearn's clone function#5525

Closed
dohmatob wants to merge 1 commit intoscikit-learn:masterfrom
dohmatob:fix-5522
Closed

[WIP] improving equality test in sklearn's clone function#5525
dohmatob wants to merge 1 commit intoscikit-learn:masterfrom
dohmatob:fix-5522

Conversation

@dohmatob
Copy link
Copy Markdown
Contributor

@dohmatob dohmatob commented Oct 22, 2015

This PR addresses issue #5522.

@dohmatob dohmatob mentioned this pull request Oct 22, 2015
Closed
ogrisel
ogrisel reviewed Oct 22, 2015
View reviewed changes
sklearn/tests/test_base.py Outdated
Copy link
Copy Markdown
Member

@ogrisel ogrisel Oct 22, 2015

Choose a reason for hiding this comment

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

I would rather raise a SkipTest('pandas required') exception to make it more explicit that this test was not run.

Copy link
Copy Markdown
Contributor Author

@dohmatob dohmatob Oct 22, 2015

Choose a reason for hiding this comment

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

yes indeed. thanks.

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Oct 22, 2015

clone would have the same problem with a numpy array passed as an input parameter right? Testing this would not require the dependency on pandas at test time.

@dohmatob
Copy link
Copy Markdown
Contributor Author

dohmatob commented Oct 22, 2015

@ogrisel: No, ndarrays are handled separately in the function.

amueller
amueller reviewed Oct 22, 2015
View reviewed changes
sklearn/tests/test_base.py Outdated
Copy link
Copy Markdown
Member

@amueller amueller Oct 22, 2015

Choose a reason for hiding this comment

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

we have a mock dataframe object.

@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 22, 2015

I am not sure I understand why this works. maybe @GaelVaroquaux can say something about it. Shouldn't we be deepcopying the dataframe? We deepcopy numpy arrays, right?

GaelVaroquaux
GaelVaroquaux reviewed Oct 22, 2015
View reviewed changes
sklearn/base.py
Copy link
Copy Markdown
Member

@GaelVaroquaux GaelVaroquaux Oct 22, 2015

Choose a reason for hiding this comment

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

The fix looks good (although it took me a minute to realize why :) )

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Oct 22, 2015

We deepcopy everything that is not an estimator (line 49 of base.py).

@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 22, 2015

yeah but why is is true after deepcopying something?

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Oct 22, 2015 via email

@amueller
Copy link
Copy Markdown
Member

amueller commented Oct 22, 2015

Argh... so both me and @GaelVaroquaux where a little unclear on what is happening here. I'll post the correct fix in a minute.

@amueller amueller mentioned this pull request Oct 23, 2015
Merged
@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Oct 23, 2015

I think #5540 is a better fix.

@ogrisel ogrisel changed the title [WIP] improving equality test inf sklearn's cloning function [WIP] improving equality test in sklearn's clone function Oct 23, 2015
@dohmatob dohmatob closed this Nov 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

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

@dohmatob @ogrisel @amueller @GaelVaroquaux