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.

Sign up for GitHub

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

Jump to bottom

Require to explicitly defining optional dimensions such as hue and markersize #7277

Merged
merged 29 commits into from Feb 11, 2023
Merged

Require to explicitly defining optional dimensions such as hue and markersize #7277

merged 29 commits into from Feb 11, 2023

Conversation

Illviljan
Copy link
Contributor

@Illviljan Illviljan commented Nov 10, 2022 •
edited

  • Matplotlib adds default colors with plt.plot(x, y) when y is a 2D array.
  • Matplotlib prioritizes c over default colors with plt.plot(x, y, c="blue") when y is a 2D array.
  • Seaborn prioritizes hue over c, but all plot axes has to be explicit.

The plot1d function now follows closer to seaborns example and require explicitly defining optional dimensions such as hue/markersize. Positives and negatives:
+++ Reliable performance, won't accidentally choose large and heavy arrays to plot.
- Loss of dimension information for default values, will essentially flatten the n-dimensional arrays.
- Other xarray plot functions guesses quite a bit as well, API is not consistent.

Required dimensions like the x-axis will still be guessed since the mpl errors weren't very intuitive.

While at it:

  • Make it possible to prioritize certain mpl kwargs when hue/markersize isn't defined.
  • Fix integer coords in plot1d and facetgrid, use .coords instead of getitem.

@Illviljan
Copy link
Contributor Author

@jorisvandenbossche, feel free to try out this PR.

@Illviljan Illviljan marked this pull request as ready for review November 12, 2022 08:18
Illviljan and others added 12 commits November 12, 2022 10:36
@dcherian
Copy link
Contributor

I disagree here, even though it runs contrary to what matplotlib does. It's better to ask users to be explicit about what they want.

We had this discussion a long time ago in one of my first PRs (!)(#1785)

@Illviljan
Copy link
Contributor Author

@dcherian, as you noted in that PR as well it's not just matplotlib but xarray too that does a lot of guessing already, x,y in plot2d for example. I was more concerned about consistency and believed we got closer to what the majority of the plotting functions does by guessing.

These are the coords that we currently guess in plot1d:

xarray/xarray/plot/dataarray_plot.py

Line 154 in d6671dd

default_guess: Iterable[str] = ("x", "hue", "size"),

Should we remove them, (empty tuple) and let the user explicitly define all of them? If so should we remove any guessing in the other plot functions as well?

@Illviljan Illviljan mentioned this pull request Nov 20, 2022
Merged
1 task
@headtr1ck headtr1ck mentioned this pull request Nov 23, 2022
Closed
4 tasks
@Illviljan Illviljan changed the title Prioritize mpl kwargs when hue/size isn't defined. Require to explicitly defining optional dimensions such as hue and markersize Nov 27, 2022
@Illviljan
Copy link
Contributor Author

This has stalled for long enough. I'll merge this at the end of next week unless someone disagrees, this pretty much reverts to original ds.plot.scatter behaviour because defining less than x and y in ds.plot.scatter was never allowed before anyway.

@Illviljan
Copy link
Contributor Author

pre-commit.ci autofix

@Illviljan Illviljan added the plan to merge Final call for comments label Feb 9, 2023
@Illviljan Illviljan merged commit 9ff932a into pydata:main Feb 11, 2023
dcherian added a commit to bzah/xarray that referenced this pull request Feb 28, 2023
@dcherian
Merge remote-tracking branch 'upstream/main' into fix/dask_indexing
97fa188 
* upstream/main: (291 commits)
  Update error message when saving multiindex (pydata#7475)
  DOC: cross ref the groupby tutorial (pydata#7555)
  [pre-commit.ci] pre-commit autoupdate (pydata#7543)
  supress namespace_package deprecation warning (doctests) (pydata#7548)
  [skip-ci] Add PDF of Xarray logo (pydata#7530)
  Support complex arrays in xr.corr (pydata#7392)
  clarification for thresh arg of dataset.dropna() (pydata#7481)
  [pre-commit.ci] pre-commit autoupdate (pydata#7524)
  Require to explicitly defining optional dimensions such as hue and markersize (pydata#7277)
  Use plt.rc_context for default styles (pydata#7318)
  Update HOW_TO_RELEASE.md (pydata#7512)
  Update whats-new for dev (pydata#7511)
  Fix whats-new for 2023.02.0 (pydata#7506)
  Update apply_ufunc output_sizes error message (pydata#7509)
  Zarr: drop "source" and  "original_shape" from encoding (pydata#7500)
  [pre-commit.ci] pre-commit autoupdate (pydata#7507)
  Whats-new for 2023.03.0
  Add `inclusive` argument to `cftime_range` and `date_range` and deprecate `closed` argument (pydata#7373)
  Make text match code example (pydata#7499)
  Remove Dask single-threaded setting in tests (pydata#7489)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcherian dcherian dcherian left review comments

Assignees
No one assigned
Labels
needs discussion needs review plan to merge Final call for comments topic-plotting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Scatter plot infers weird default values
3 participants
@Illviljan @dcherian @headtr1ck