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

[WIP] Modify pairplot to include jointplot and cornerplot like features #1079

Merged
merged 12 commits into from
Mar 5, 2020
Merged

[WIP] Modify pairplot to include jointplot and cornerplot like features #1079

merged 12 commits into from
Mar 5, 2020

Conversation

agustinaarroyuelo
Copy link
Contributor

@agustinaarroyuelo agustinaarroyuelo commented Feb 18, 2020

The motivation behind this PR is that plots that are currently made with plot_joint can be made with plot_pair. Also, the point_estimate key word argument and scatter_kde kind will allow pair plots similar to fairly used cornerplots.
Here are some examples:
pairplot+jointplot_4
pairplot+jointplot3

I will really appreciate suggestions, especially on function testing.

Checklist

  • Follows official PR format
  • Includes a sample plot to visually illustrate the changes (only for plot-related functions)
  • New features are properly documented (with an example if appropriate)?
  • Includes new or updated tests to cover the new feature
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@ahartikainen
Copy link
Contributor

Looks good. Not needed for this PR, but it could be interesting to have this property for bokeh, but for any point that is selected.

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This is a kind of plot that has been missing far too much time.

I have commented on some aspects of the code, my main motivations being reusing code when possible (it's less code to maintain on the long run), using sensible default while letting users customize most if not all features and last but not least things I happened to see by chance that I missed in the past. Feel free to not follow all of them. I can help with testing too afterwards.

Also, general note, I'd be great if a second pair of eyes could confirm (either you or another reviewer), the **kwargs in plot_kde are passed to the backend but NOT accepted by neither of them. It looks safe to delete (it actually feels safer than having it like this).

arviz/plots/backends/bokeh/distplot.py Outdated Show resolved Hide resolved
arviz/plots/plot_utils.py Show resolved Hide resolved
arviz/plots/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/matplotlib/pairplot.py Show resolved Hide resolved
@agustinaarroyuelo
Copy link
Contributor Author

Thanks Oriol for your comments! I am going through them 👍

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

github marks the changes in a very strange way, I think I commented on green things that have been on the code for quite a long time, there is no need to fix all these extra comments.

CHANGELOG.md Outdated Show resolved Hide resolved
arviz/data/io_pymc3.py Outdated Show resolved Hide resolved
arviz/plots/backends/bokeh/pairplot.py Outdated Show resolved Hide resolved
arviz/plots/backends/bokeh/pairplot.py Show resolved Hide resolved
arviz/plots/backends/bokeh/pairplot.py Show resolved Hide resolved
@ahartikainen
Copy link
Contributor

I recommend that you rebase your branch.

First update your master against arviz master.

Then go to your branch

git rebase -i master

And then you can remove unneeded commits and keep only your.

rebase -i should open a text editor (vim for example) and then you can edit commits as needed.

@agustinaarroyuelo
Copy link
Contributor Author

Thank you Ari for your help.
I did what you suggested and removed the commits that were previous to rebasing. This commits were duplicated. When I tried to push this to my branch, the tip was behind its remote counterpart. Git suggested git pull ..., but pulling from my branch would have reverted the changes made by git rebase -i master. Therefore I forced the push, using --force-with-lease just in case. Let me know if it is ok.

@ahartikainen
Copy link
Contributor

git push -f is needed, I forgot to add that.

@@ -68,13 +76,29 @@ def plot_pair(
Matplotlib axes or bokeh figures.
divergences_kwargs : dicts, optional
Additional keywords passed to ax.scatter for divergences
plot_kwargs : dicts, optional
Additional keywords passed to ax.plot, az.plot_kde or ax.hexbin
scatter_kwargs:
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the changes parts of the docstring are outdated. For example

kind : str
    Type of plot to display (scatter, kde or hexbin)

is not an "or" but and/or. And probably we should explicitly indicate the overlaying feature.

Instead of
Plot a scatter or hexbin matrix of the sampled parameters.

we should have something like:

Scatter, kde and/or hexbin matrix with (optional) marginals on the diagonal

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the examples need to be updated. It's OK for me if we do that in another PR (but we should open and issue so we do not forget).

run pylint
@OriolAbril
Copy link
Member

Changelog should be updated to list changes in unreleased version, not 0.7.0

Copy link
Member

@OriolAbril OriolAbril left a comment

Choose a reason for hiding this comment

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

I think we can merge and work on several issues/ideas that came up thanks to this in other PRs, I'll try to create some issues so we don't forget. Thanks for the PR!

@ahartikainen ahartikainen merged commit 3081799 into arviz-devs:master Mar 5, 2020
@agustinaarroyuelo agustinaarroyuelo deleted the pairplot_jointplot branch March 5, 2020 12:13
@agustinaarroyuelo
Copy link
Contributor Author

Thanks everyone for your comments and help!

OriolAbril pushed a commit that referenced this pull request Mar 5, 2020
…es (#1079)

* add jointplot features into pairplot

* add scatter_kde kind for pairplot

* add point_estimate arguments

* bokeh backend

* fix None argument for color in kdeplot bokeh backend

* run black, pylint and pytest

* remove scatter_kde kind among several other changes

* minor changes

* run pytest

* add plot width and height to backend_kwargs

fix pylint issues

fix hover feature

fix hover feature

minor fixes

* update docstring

run pylint

* update changelog
aloctavodia pushed a commit that referenced this pull request Apr 13, 2020
* [WIP] Modify pairplot to include jointplot and cornerplot like features (#1079)

* add jointplot features into pairplot

* add scatter_kde kind for pairplot

* add point_estimate arguments

* bokeh backend

* fix None argument for color in kdeplot bokeh backend

* run black, pylint and pytest

* remove scatter_kde kind among several other changes

* minor changes

* run pytest

* add plot width and height to backend_kwargs

fix pylint issues

fix hover feature

fix hover feature

minor fixes

* update docstring

run pylint

* update changelog

* add true_values argument for plot_pair

remove whitespace

* change argument name to reference_values

change argument name to reference_values

* update tests

Co-authored-by: Ari Hartikainen <[email protected]>
canyon289 pushed a commit that referenced this pull request May 11, 2020
* [WIP] Modify pairplot to include jointplot and cornerplot like features (#1079)

* add jointplot features into pairplot

* add scatter_kde kind for pairplot

* add point_estimate arguments

* bokeh backend

* fix None argument for color in kdeplot bokeh backend

* run black, pylint and pytest

* remove scatter_kde kind among several other changes

* minor changes

* run pytest

* add plot width and height to backend_kwargs

fix pylint issues

fix hover feature

fix hover feature

minor fixes

* update docstring

run pylint

* update changelog

* fix jointplot with bokeh

* To transform colors in plotting functions to hex (#1084)

* Transform colors to hex in plot_khat

* Reformatted with black

* Added hex conversion to khatplot.py and vectorized_to_hex to plot_utils.py

* Black changes

* Added keep_alpha parameter and list comprehension

* Pydocstyle changes

* More pydocstyle changes

* generalised vectorized_to_hex

* Black changes

* Black changes

* Modified khatplot hline_kwargs, vectorized_to_hex and test for vectorized_to_hex

* Black changes

* Rewrote tests and modified vectorized_to_hex

* lint and minor fixes

Co-authored-by: Oriol (ZBook) <[email protected]>

* [WIP] Modify pairplot to include jointplot and cornerplot like features (#1079)

* add jointplot features into pairplot

* add scatter_kde kind for pairplot

* add point_estimate arguments

* bokeh backend

* fix None argument for color in kdeplot bokeh backend

* run black, pylint and pytest

* remove scatter_kde kind among several other changes

* minor changes

* run pytest

* add plot width and height to backend_kwargs

fix pylint issues

fix hover feature

fix hover feature

minor fixes

* update docstring

run pylint

* update changelog

* fix jointplot with bokeh

* update changelog

* fixed changelog

Co-authored-by: Ari Hartikainen <[email protected]>
Co-authored-by: amukh18 <[email protected]>
Co-authored-by: Oriol (ZBook) <[email protected]>
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.

4 participants