issue_comments

Yeah! It owes a lot to your hard work too. Hopefully people find this useful.

Merging.

Yay, tests pass. I'll merge in a few days (cc @pydata/xarray, @yohai )

I don't know what to do about this test failure:

``` xarray/tests/test_plot.py .............................................. [ 92%] ........................................................................ [ 92%] ........................................................................ [ 93%] ...s.................................................................... [ 94%] .........................Xx INTERNALERROR> Traceback (most recent call last): INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/main.py", line 213, in wrap_session INTERNALERROR> session.exitstatus = doit(config, session) or 0 INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/main.py", line 257, in _main INTERNALERROR> config.hook.pytest_runtestloop(session=session) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in call INTERNALERROR> return self._hookexec(self, self.get_hookimpls(), kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec INTERNALERROR> return self._inner_hookexec(hook, methods, kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda> INTERNALERROR> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 203, in _multicall INTERNALERROR> gen.send(outcome) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result INTERNALERROR> raise ex[1].with_traceback(ex[2]) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall INTERNALERROR> res = hook_impl.function(args) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/main.py", line 278, in pytest_runtestloop INTERNALERROR> item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in call INTERNALERROR> return self._hookexec(self, self.get_hookimpls(), kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec INTERNALERROR> return self._inner_hookexec(hook, methods, kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda> INTERNALERROR> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 208, in _multicall INTERNALERROR> return outcome.get_result() INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result INTERNALERROR> raise ex[1].with_traceback(ex[2]) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall INTERNALERROR> res = hook_impl.function(args) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/runner.py", line 72, in pytest_runtest_protocol INTERNALERROR> runtestprotocol(item, nextitem=nextitem) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/runner.py", line 87, in runtestprotocol INTERNALERROR> reports.append(call_and_report(item, "call", log)) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/runner.py", line 171, in call_and_report INTERNALERROR> hook.pytest_runtest_logreport(report=report) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/hooks.py", line 289, in call INTERNALERROR> return self._hookexec(self, self.get_hookimpls(), kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 87, in _hookexec INTERNALERROR> return self._inner_hookexec(hook, methods, kwargs) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/manager.py", line 81, in <lambda> INTERNALERROR> firstresult=hook.spec.opts.get("firstresult") if hook.spec else False, INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 208, in _multicall INTERNALERROR> return outcome.get_result() INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 80, in get_result INTERNALERROR> raise ex[1].with_traceback(ex[2]) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/pluggy/callers.py", line 187, in _multicall INTERNALERROR> res = hook_impl.function(*args) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/junitxml.py", line 592, in pytest_runtest_logreport INTERNALERROR> reporter.append_skipped(report) INTERNALERROR> File "/usr/share/miniconda/envs/xarray-tests/lib/python3.7/site-packages/_pytest/junitxml.py", line 250, in append_skipped INTERNALERROR> if xfailreason.startswith("reason: "): INTERNALERROR> AttributeError: 'list' object has no attribute 'startswith'

= 7676 passed, 322 skipped, 25 xfailed, 5 xpassed, 41 warnings in 270.39 seconds =

[error]Bash exited with code '1'.

```

Tests seem to be failing due to lint errors, see https://github.com/pydata/xarray/pull/2277#issuecomment-447607250

Maybe just read over the docs and doc strings?

I haven't reviewed this in detail, but I wonder if we shouldn't just merge it in. This has tests and docs, and most of this has been looked over by both @dcherian and @yohai. Also, it's in a pretty self-contained part of the code (plotting). So it seems pretty slow risk to me.

1/2 AppVeyor tests seems to have crashed randomly.

Failed test is #2803

I went with markersize but am happy to change it to mark_size or something else.

I think I'm done here. This is missing a legend that shows the size variation, but otherwise it's complete. I'd like to leave the legend to a future PR if that's OK.

Could we call this mark_size instead of scatter_size? The later sounds a little awkward to me.

OK PR switched to add_guide, this might need some reworking once we add other plotting methods for which add_colorbar is more sensible.

Phew! added support for scatter_size now.

python ds = xr.tutorial.scatter_example_dataset() ds.plot.scatter(x='A', y='B', col='z', scatter_size='z')

Well I can't think of anything better than guide so shall we proceed with that?

I'm not sure about this. Legends and colorbars are different and the DataArray.plot API has both switches: add_legend and add_colorbar.

Is there a generic term that covers both discrete legends and colorbars?

ggplot2 uses "guide" but I haven't seen that elsewhere. Still, add_guide=False could be a good way to write this genericly.

For non-numerical hues (or when hue_style='discrete') the legend is turned off by add_legend=False but for numerical ones by add_colorbar=False, which might be not intuitive and might require manual editing in various places if you change your mind between a discrete and continuous legend. I would suggest to either unify both options to one, or make them interchangeable (if not contradicting).

I'm not sure about this. Legends and colorbars are different and the DataArray.plot API has both switches: add_legend and add_colorbar.

@pydata/xarray what do you all think?

Ok this is ready for review. I haven't added size yet. I think we will need to go through a deprecation cycle before doing that.

One alternative would be to have size not control axis height in the scatter method only. I'm not a big fan of this approach though .

This generally looks good to me...

I guess we still use s= for indicating marker size?

Yeah. We'll need to go through a deprecation cycle to rename size to height.

This generally looks good to me...

I guess we still use s= for indicating marker size?

I think this is ready for a preliminary review.

I've made the following changes:

1. Add a hue_style kwarg that can be either 'discrete' (for legend) or 'continuous' (colorbar). add_legend, add_colorbar turn on/off displaying the legend/colorbar which is consistent with our existing DataArray.plot API.

   Examples: ds.plot.scatter(x='A', y='B', hue='x', hue_style='discrete') ds.plot.scatter(x='A', y='B', hue='x', hue_style='continuous')

2. Under the hood, scatter is always used instead of alternating between plot, scatter depending on hue_style.

3. Everything has been refactored out to plot/dataset_plot.py

Do people like hue_style? Or something else?

@yohai thanks; I might make some changes in the next few weeks.

@yohai do you have time to pick this back up?

Not so out of topic: https://twitter.com/michaelwaskom/status/1021428306326024193

So, let's say it: your work is highly appreciated @yohai and @dcherian ! Writing a comprehensive and (at the same time) easy to use plotting API is hard...

The reason I started this PR in the first place is that I happened to do relatively simple scatter plots quite often, so I thought it'd be handy. but for more elaborate ones I would use a dedicated tool like seaborn.

:+1: This seems like a reasonable place to draw the line to me.

I like this!

As always for plotting functionalities, one or more examples in the gallery would be welcome! This can be done in a subsequent PR of course.

This is looking really nice. Coincidentally, the new version of Seaborn was released today, and has a whole new doc section on "relational plots": http://seaborn.pydata.org/tutorial/relational.html#relational-tutorial

It's probably worth a look over to see if it has good ideas worth stealing, or if we want to make intentional deviations from its behavior in xarray.

It is possibly worth taking a look at the recent (not yet released) scatterplot (https://github.com/mwaskom/seaborn/pull/1436) and relplot (https://github.com/mwaskom/seaborn/pull/1477) additions to Seaborn.

seaborn.scatterplot will use hue/size rather than c/s, which is definitely more readable. One hazard is that it it means that the size argument from seaborn.FacetGrid needs to be renamed to avoid name conflicts -- it's now becoming height. Unfortunately we would also need to rename the size argument if we followed Seaborn's example.

I guess I can see the virtue in sticking with matplotlib's old c/s names, but those really are terrible names. Maybe hue/mark_size would be a good compromise? Or we could systematically switch size -> height elsewhere like Seaborn.

You should try to emulate the pandas scatter api as much as possible (which itself emulates the matplotlib api). That means using the c keyword instead of hue and also implementing s for size.

I disagree here. In many cases, we already follow the naming conventions from Seaborn instead, which uses meaningful names. c and s are pretty meaningless.

This seems like a very cool and useful feature! A few comments:

• This should probably live under the .plot namespace, i.e. da.plot.scatter rather than da.scatter.
• You should try to emulate the pandas scatter api as much as possible (which itself emulates the matplotlib api). That means using the c keyword instead of hue and also implementing s for size

Happy to provide a more detailed review once the tests are implemented.