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 examples to customize 2D KDE #1158

Merged
merged 4 commits into from
Apr 21, 2020
Merged

add examples to customize 2D KDE #1158

merged 4 commits into from
Apr 21, 2020

Conversation

aloctavodia
Copy link
Contributor

@aloctavodia aloctavodia commented Apr 20, 2020

Following #1157 this add examples to the docstring of plot_kde and a new example to the gallery.
This also sets fill_last=False as the default, because nobody likes the previous setting :-)

  • Follows official PR format
  • Code style correct (follows pylint and black guidelines)
  • Changes are listed in changelog

@OriolAbril
Copy link
Member

LGTM, and I completely agree with the fill_last=False default 🎉

@codecov
Copy link

codecov bot commented Apr 20, 2020

Codecov Report

Merging #1158 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1158      +/-   ##
==========================================
- Coverage   93.12%   93.11%   -0.02%     
==========================================
  Files          94       94              
  Lines        9286     9289       +3     
==========================================
+ Hits         8648     8649       +1     
- Misses        638      640       +2     
Impacted Files Coverage Δ
arviz/plots/kdeplot.py 100.00% <ø> (ø)
arviz/data/converters.py 67.14% <0.00%> (-1.43%) ⬇️
arviz/plots/pairplot.py 92.94% <0.00%> (-0.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a341b66...cebebc0. Read the comment docs.

@rpgoldman
Copy link
Contributor

See my feature request #1159 -- we should show usage of the cmap property to control the color scheme. Understanding the effect of cmap turns out to be critical to getting attractive plots.

@aloctavodia
Copy link
Contributor Author

Two of the new examples show how to pass a cmap argument.

Copy link
Contributor

@rpgoldman rpgoldman left a comment

Choose a reason for hiding this comment

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

Suggest we briefly explain what the contour_kwargs and contourf_kwargs are for.

I.e., add to the docstring:
"Contour keyword arguments control the formatting of the contour lines." and "Contourf keyword arguments control the filling of contours."

In general -- and I don't think this is ArviZ's fault -- use of these keyword arguments requires a lot of understanding of the underlying implementation. This would be OK if we could leave it to Arviz to choose defaults and be happy with the results, but it seems like we still need a bunch of user input.
Maybe this could be fixed by choosing a good default colormap for the users, so that at least they don't have to understand, instead of leaving it unpredictably to the state of the environment?

@OriolAbril
Copy link
Member

Maybe this could be fixed by choosing a good default colormap for the users, so that at least they don't have to understand, instead of leaving it unpredictably to the state of the environment?

We should probably extend ArviZ native style like arviz-darkgrid and so one to be more explicit on the defaults (now they basically are designed to be called on top of matplotlib default and only list the parameters to override), however, for most arguments (at least in matplotlib, which is what I know better) we want the output to depend on matplotlib rcparams. As I see the situation, there are several possibilities, and the least bad is the current setting:

  • We do not set the default in ArviZ to use matplotlib defaults. This has the advantage of letting users do plt.rcparams["cmap"] = "plasma" and have plasma be the default in native matplotlib, seaborn, ArviZ... (basically all other libraries on top of matplotlib that do not override the default). The main drawback is dependence on previous state as you said, but extending ArviZ styles should fix this.
  • We use an ArviZ rcparam. It is similar to the previous situation, with the main difference that matplotlib defaults no longer affect ArviZ both for the good and for the bad, getting all plots to use the same cmap would require 2 commands (if we use both matplotlib and ArviZ). The main con I see is that if all libraries on top of matplotlib do that, defaults will escalate quickly.
  • We set the default in the plotting functions. This hardcodes the default which would then be impossible to override at a general level.

Would the first situation (which is basically the current one) work for your use case if styles were to guarantee reproducibility of results? I hope outlining the alternatives helps in understanding why plots behave like they do.

@aloctavodia
Copy link
Contributor Author

Using styles you can also do plt.rcparams["cmap"] = "plasma" if you want, styles overwrite the default rcParams and can be overwritten to. Setting values by default in the arviz style do not prevent they to be overwritten, so is not a general solution to the issue Robert is seeing, which is just the combination of two styles.

Copy link
Contributor

@rpgoldman rpgoldman left a comment

Choose a reason for hiding this comment

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

Thanks! I think that will be helpful.

@aloctavodia
Copy link
Contributor Author

Thanks @rpgoldman for bringing up the issue.

@aloctavodia aloctavodia merged commit 6a51574 into master Apr 21, 2020
@aloctavodia aloctavodia deleted the kde_2d_examples branch April 21, 2020 17:13
nitishp25 added a commit to nitishp25/arviz that referenced this pull request Apr 27, 2020
* added groups

* update tests and changelog

* lint changes

* update from_dict and tests

* update changelog

* modify io_dict

* minor fixes

* update changelog again

Add warning for log scale default in compare/loo/waic functions (arviz-devs#1150)

* Added scale warning to ELPDData

* Added scale warning to compare function

* Added changes to Changelog

* Moved warning to end of string in loo function

* Removed last test for loo_print

* Ran Black

* Integrated Oriol's comments

add local to docstring, and use lowercase for ess (arviz-devs#1152)

hardcode show=False in plot_posterior subplots (arviz-devs#1151)

* hardcode show=False in plot_posterior subplots

* update changelog

Fix documentation and deprecation warning in pair plot (arviz-devs#1156)

The "kind" argument is not described correctly.

* Fix pairplot warning.

Previously, because of incorrect argument checking, pairplot would
mistakenly warn the caller not to use the "contour" argument when that
argument was NOT supplied.  Changed default value to None and did the
defaulting by hand to fix this issue.

Also added some type declarations.

* Clarified docstring for contour argument.

Co-authored-by: Robert P. Goldman <rpgoldman@sift.net>

add viridis as default cmap (arviz-devs#1160)

add examples to customize 2D KDE (arviz-devs#1158)

* add examples to customize 2D KDE

* blackify gallery example

* update changelog

* briefly explain contour_kwargs and contourf_kwargs
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