Skip to content

ENH add vlines_ attribute to PDP Display to hide deciles - #15785

Merged
rth merged 11 commits into
scikit-learn:masterfrom
NicolasHug:vlines
Apr 19, 2020
Merged

ENH add vlines_ attribute to PDP Display to hide deciles#15785
rth merged 11 commits into
scikit-learn:masterfrom
NicolasHug:vlines

Conversation

@NicolasHug

Copy link
Copy Markdown
Member

I added a vlines_ attribute to the PDP Display so that we can hide the deciles vertical lines. But maybe there's a way to access the lines via the ax? I couldn't find anything.

@thomasjpfan please LMK if it's worth it, and I'll add test or close

@thomasjpfan

Copy link
Copy Markdown
Member

There is a way to access everything through ax. Since anyone can draw lines onto the axes, having the artist in the display object specifics which lines were draw by the display object.

I think adding vlines is a good idea.

lines_ravel = self.lines_.ravel(order='C')
contours_ravel = self.contours_.ravel(order='C')

vlines = np.empty_like(self.axes_).ravel()

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.

To keep with the style of the surrounding code:

self.vlines_ = np.empty_like(self.axes_)
vlines_ravel = self.vlines_.ravel(order='C')

(Yes the order is C by default, but I recall a comment wanting this to be explicit)

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.

Given #15788, we should specific the dtype as object as well.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

OK I'll make it explicit, though note thatempty_like will explicitly set the dtype to that of axes_

@NicolasHug NicolasHug changed the title [WIP] add vlines_ attribute to PDP Display to hide deciles [MRG] add vlines_ attribute to PDP Display to hide deciles Dec 4, 2019
@NicolasHug

NicolasHug commented Dec 4, 2019

Copy link
Copy Markdown
Member Author

Thanks for the feedback. ready for actual reviews now. I also realized we would need a hlines attribute for 2-way PDPs

@thomasjpfan thomasjpfan left a comment

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.

LGTM! Needs whats new in 0.23

Comment thread sklearn/inspection/tests/test_plot_partial_dependence.py
@NicolasHug NicolasHug self-assigned this Apr 8, 2020
@NicolasHug NicolasHug added this to the 0.23 milestone Apr 8, 2020
@NicolasHug

Copy link
Copy Markdown
Member Author

Easy one @rth if you have time?

@rth

rth commented Apr 19, 2020

Copy link
Copy Markdown
Member

Sounds good. Could you point me to an example of PDP plot with deciles that we would be hiding?

@NicolasHug

NicolasHug commented Apr 19, 2020

Copy link
Copy Markdown
Member Author

here you have plots with deciles https://scikit-learn.org/stable/modules/partial_dependence.html which are the black vertical bars on the 1-way plots and the the 2-way plot also has horizontal bars

here is an example without deciles (first plot) http://nicolas-hug.com/blog/pdps

@rth

rth commented Apr 19, 2020

Copy link
Copy Markdown
Member

which are the black vertical bars on the 1-way plots and the the 2-way plot also has horizontal bars

The ticks on the axis, right? I guess you can always change ax.xaxis.set_ticks but indeed I don't know how to remove some and not the others.

@rth rth left a comment

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.

Ahh, I didn't get that you are drawing them manually, so they are not part of the ax.xaxis / ax.yaxis objects.

LGTM.

@rth rth changed the title [MRG] add vlines_ attribute to PDP Display to hide deciles ENH add vlines_ attribute to PDP Display to hide deciles Apr 19, 2020
@rth
rth merged commit b4757f7 into scikit-learn:master Apr 19, 2020
@NicolasHug

Copy link
Copy Markdown
Member Author

thanks!

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 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