issue_comments

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

Thanks @dcherian ! Glad to see this finally merged

Merging.

Hello @yohai! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-08-07 18:39:42 UTC

Thanks for this

On Mon, Aug 5, 2019 at 12:04 PM Deepak Cherian notifications@github.com wrote:

Yay, tests pass. I'll merge in a few days (cc @pydata/xarray https://github.com/orgs/pydata/teams/xarray, @yohai https://github.com/yohai )

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/2277?email_source=notifications&email_token=ABPA5PNDIYQDJME4CUJREDDQDBFQNA5CNFSM4FJJSKRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3SJCVA#issuecomment-518295892, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPA5PMUANDUGDQKZHTIPYDQDBFQNANCNFSM4FJJSKRA .

-- Yohai Bar Sinai Post Doctoral Fellow John A. Paulson School of Engineering and Applied Sciences Harvard University

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'.

```

@dcherian @shoyer I think it's ready to merge

turns out it was an easy fix. But now I wonder if we somehow screwed up other plotting functionalities without noticing (I checked a few but found nothing)

Hold the press! we can't merge. It seems like the new functionality messes up the faceted lineplots. If we define ds=xr.tutorial.scatter_example_dataset() then current behavior is :

ds.A.plot(col='x', row='w', hue='z'):

but with the pull request we screw up the legend

I need to look at this.

Also - kinda unsettling that this was not picked up by any unit test.

@dcherian @shoyer I can't seem to build the docs on my machine because of cartopy or something. Is there a way to access a built version from travis or something like that?

I'll try to have a look tonight

On Thu, Jun 20, 2019, 11:30 Stephan Hoyer notifications@github.com wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/pydata/xarray/pull/2277?email_source=notifications&email_token=ABPA5POP2JLG6ETE45NGEWDP3OPDBA5CNFSM4FJJSKRKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYFYT2Y#issuecomment-504072683, or mute the thread https://github.com/notifications/unsubscribe-auth/ABPA5PN4I5RBWUDFYBCAZU3P3OPDBANCNFSM4FJJSKRA .

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?

Anything I can do?

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.

I think matplotlib uses markersize

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')

My feeling is that "legend" applies to both a color bar and a discrete legend (or other more complicated stuff too) but I am not a native English speaker. I'm fine with whichever decision.

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?

This looks great! Thanks @dcherian!

The only comment I have is with respect to the part of the API that specifies whether a legend should be drawn or not. 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).

something like:

python if xor(add_legend, add_colorbar): #decide according to plot type: if it's non-numeric or `hue_style='discrete'` # then use value of add_legend, else use value of add_colorbar elif add_legend or add_colorbar: #plot legend according to current logic

but really there should only be one allowable keyword here I 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.

@dcherian thanks for nudging. Actually I'm in a pretty stressed time now. I will be able to pick this up in a few months, but I wouldn't mind if someone else does it in the meantime (I don't think there's a lot of work there)

@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.

thanks @dcherian for the review. I'll fix the code when I get to it later this week,

Regarding the size and marker style - I'm not sure it makes sense to duplicate this functionality. Is it not easier to stack the two data arrays into one, use to_pandas and then just use seaborn directly? 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.

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.

I don't have an opinion about naming variables and would be happy with whatever decision y'all make.

For the code -- I added tests and changed the logic a bit. Following @dcherian's suggestion, now the default behavior is no longer coloring hues with discrete values (legend) but rather with a continuous scale (colorbar). It does make actually more sense and I think it should also be the default behavior for regular line plots. This is the API now:

python A = xr.DataArray(np.zeros([3, 20, 4, 4]), dims=[ 'x', 'y', 'z', 'w'], coords=[np.sort(np.random.randn(k)) for k in [3,20,4,4]]) ds=xr.Dataset({'A': A.x+A.y+A.z+A.w, 'B': -0.2/A.x-2.3*A.y-np.abs(A.z)**0.123+A.w**2}) ds.A.attrs['units'] = 'Aunits' ds.B.attrs['units'] = 'Bunits' ds.z.attrs['units'] = 'Zunits' ds.plot.scatter(x='A', y='B')

Specifying hue creates a colorbar: python ds.plot.scatter(x='A',y='B', hue='z') If, however, the hue dimension is not numeric, then a legend is created: python ds['z']= ['who', 'let','dog','out'] ds.plot.scatter(x='A',y='B', hue='z')

If you want a discrete legend even for numeric hues, you can specify it explicitly: python ds.plot.scatter(x='A',y='B', hue='w', discrete_legend=True)

I am a bit bothered by the fact that this is not only a different coloring method, it's a very different style altogether (under the hood using plot instead of scatter). I don't know if it's a good thing or a bad thing that the same function can produce very different looking figures. Input will be welcome about that.

Of course, faceting works as you think it should: python ds.plot.scatter(x='A',y='B', hue='z',col='x') ds.plot.scatter(x='A',y='B', hue='w',col='x', col_wrap=2)

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.

Could not review pull request. It may be too large, or contain no reviewable changes.

Could not review pull request. It may be too large, or contain no reviewable changes.