Skip to content

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

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

ENH add vlines_ attribute to PDP Display to hide deciles#15785
rth merged 11 commits intoscikit-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
Copy link
Copy Markdown
Member Author

NicolasHug commented Dec 4, 2019

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

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.

LGTM! Needs whats new in 0.23

@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
Copy link
Copy Markdown
Member

rth commented Apr 19, 2020

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

@NicolasHug
Copy link
Copy Markdown
Member Author

NicolasHug commented Apr 19, 2020

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
Copy link
Copy Markdown
Member

rth commented Apr 19, 2020

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.

Copy link
Copy Markdown
Member

@rth rth left a comment

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