Skip to content

TST Replace Boston dataset in test_plot_partial_dependence#17144

Merged
glemaitre merged 3 commits intoscikit-learn:masterfrom
lucyleeow:test_plt_part_dep
May 20, 2020
Merged

TST Replace Boston dataset in test_plot_partial_dependence#17144
glemaitre merged 3 commits intoscikit-learn:masterfrom
lucyleeow:test_plt_part_dep

Conversation

@lucyleeow
Copy link
Copy Markdown
Member

@lucyleeow lucyleeow commented May 6, 2020

Reference Issues/PRs

Towards #16155

What does this implement/fix? Explain your changes.

Replaces Boston dataset with diabetes dataset in test_plot_partial_dependence.py.

Any other comments?

Col 1 of diabetes is not quantitative (sex), thus used columns 0 & 2 instead of 0 & 1.
feature_names of Boston is of type numpy.ndarray while for diabetes it is a list. Amended diabetes feature_names to type numpy array (in this test) instead of changing the original tests.

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you @lucyleeow !

def diabetes():
diabetes = load_diabetes()
# Match type of `feature_names` of prev used `load_boston`
diabetes.feature_names = np.asarray(diabetes.feature_names)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there somewhere in this file that requires this to be a ndarray?

Copy link
Copy Markdown
Member Author

@lucyleeow lucyleeow May 9, 2020

Choose a reason for hiding this comment

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

I decided to change the type of feature_names instead of changing the test, but I can change the test instead:

Also test_plot_partial_dependence_custom_axes specifically converts feature_names to a list but I didn't understand what it is testing/why this would make a difference. Thus I didn't want to change the original type of feature_names in case it makes a difference that I don't understand (if that makes sense).

@lucyleeow
Copy link
Copy Markdown
Member Author

thanks @thomasjpfan!

@lucyleeow
Copy link
Copy Markdown
Member Author

ping @glemaitre

@glemaitre glemaitre merged commit 049d71b into scikit-learn:master May 20, 2020
@glemaitre
Copy link
Copy Markdown
Member

Thanks @lucyleeow

@lucyleeow lucyleeow deleted the test_plt_part_dep branch May 20, 2020 13:34
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
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.

3 participants