Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scatter matrix reference #689

Merged
merged 3 commits into from
Mar 23, 2022
Merged

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Dec 16, 2021

This PR adds a reference page for the scatter_matrix plots. It also fixes to docstring of this function to reflect the fact that tools only support a list of tools.

@maximlt maximlt force-pushed the maximlt_scatter_matrix_reference branch from f1be9b8 to 4562221 Compare February 21, 2022 17:23
@maximlt
Copy link
Member Author

maximlt commented Feb 21, 2022

Could you look into this @jlstevens please?

The only thing I'm unsure about this reference notebook is that I've integrated the docstring by copy pasting it and reformatting it so that it looks better in markdown. Maybe there's a way (a directive?) to have it automatically both pulled from the code base and nicely rendered.

@jbednar
Copy link
Member

jbednar commented Feb 21, 2022

My vote is for Param and HoloViews to support an HTML rendering of the docs for a Parameterized object, which is something I sketched at holoviz/param#425, and then that could be used here and throughout the docs.I got stuck when realizing that HoloViews' own HTML repr was being overridden by this support, but it's probably not too big a job to fix that and then improve the representation so that it works well both as docs and in Jupyter, just like xarray does. Seems like an easy win and a major step forward in usability!

@maximlt
Copy link
Member Author

maximlt commented Feb 21, 2022

Oh yes that would be great except that in this particular case scatter_matrix is a bare Python function so it'd need to be turned into a parameterized object (a ParameterizedFunction object maybe) first and then use the new HTML repr.

@jbednar
Copy link
Member

jbednar commented Feb 21, 2022

Good point, but yes, a ParameterizedFunction then, so that all documented stuff could have a similar repr.

@jlstevens
Copy link
Collaborator

@maximlt I'll have a look at this shortly.

@jlstevens
Copy link
Collaborator

jlstevens commented Feb 22, 2022

Looks great! Two comments:

  1. How about a minor edit/elaboration at the start:

    scatter_matrix shows all the pairwise relationships between the columns of your data. Each non-diagonal entry plots the corresponding columns against another, while the diagonal plot shows the distribution of the data within each individual column.

  2. I like the parameters list but I think it is important to only focus on those specific to scatter_matrix. People need to be reminded that many options are generic and apply to all/most plot types in hvplot. I would simply list what is most important for scatter_matrix(data, chart and diagonal i.e. the unique kind options) and then remind people of the generic options and the datashading options (i.e. mention them with a link).

    I have noticed that one reason you have probably done it this way is that the behavior of the generic options sometimes applies to the diagonal or off-diagonal plots (or both). It looks to me like all the generic options apply to the off-diagonal plots unless listed in diagonal_kwds/hist_kwds/density_kwds. Is this correct? If so, I would just state that...

@maximlt
Copy link
Member Author

maximlt commented Feb 25, 2022

Thanks for your review Jean-Luc! I'll make the small edit you suggest at the start. On your second point, I've actually just exposed in the docs the parameters that are in the function signature and documented in the docstring. But thinking about it more this doesn't seem like a good long-term solution, I've seen in Panel how difficult it is to properly sync the Parameters doc (in the code) with the parameters description (in the reference gallery, manually typed) of its components. So until we get a better repr for Parameterized objects or an API reference for hvplot, I would instead either not show the parameters description (remove what I've added) or just display something like print(scatter_matrix.__doc__). What do you think?

@maximlt
Copy link
Member Author

maximlt commented Mar 23, 2022

I'm going ahead and merging. The last change I've made updates the intro as suggested in the review. I've removed the parameters description, even if it was somehow nice to have I believe it wasn't the right place and time to do this change. This should happen across the whole documentation once a better system to document the API parameters is found.

@maximlt maximlt merged commit 0ec9337 into master Mar 23, 2022
@maximlt maximlt deleted the maximlt_scatter_matrix_reference branch March 23, 2022 09:11
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.

3 participants