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 a plotting example #61

Merged
merged 12 commits into from
Feb 20, 2021
Merged

Conversation

keewis
Copy link
Collaborator

@keewis keewis commented Feb 10, 2021

towards #2

We are currently lacking examples. This adds a plotting example using xarray's tutorial dataset (air_temperature), but since pint-xarray is domain agnostic, it would be cool to have examples from different domains.

"metadata": {},
"outputs": [],
"source": [
"monthly_means.pint.dequantify(format=\"~P\").plot.imshow(col=\"month\", col_wrap=4)"

Choose a reason for hiding this comment

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

I think it would be totally fine to modify xarray.plot.utils.label_from_attrs to do this for pint arrays with some reasonable-default choice of format.

Copy link
Collaborator Author

@keewis keewis Feb 10, 2021

Choose a reason for hiding this comment

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

indeed. However, I would like to keep the tight coupling to a minimum. Maybe hooks or entrypoints would help?

For now, I think asking people to dequantify before plotting / saving the data is fine.

Copy link
Member

@TomNicholas TomNicholas Jul 1, 2021

Choose a reason for hiding this comment

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

@keewis I feel like this is an example of "you ain't gonna need it". I would be in favour of special-casing xarray.plot.utils.label_from_attrs until such a time that we actually need an entrypoint for other array libraries. After all, xarray's plotting methods already special-case dask arrays by calling .compute(), it's not that big of a deal if they call .dequantify() too, surely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue with hard-coding is that we do expect more duck arrays in the future (e.g. pytorch and maybe uncertainties), and adding special cases for all of them would quickly become impossible.

I was hoping to make use of matplotlib.units for this. For example, using:

In [9]: ureg = pint.UnitRegistry()
   ...: ureg.setup_matplotlib()
   ...:
   ...: t = ureg.Quantity(np.arange(10), "s")
   ...: v = ureg.Quantity(5, "m / s")
   ...: x = v * t
   ...: 
   ...: fig, ax = plt.subplots(1, 1)
   ...: ax.plot(t, x)
   ...: 
   ...: plt.show()

would be pretty nice. However, while investigating #91 I found that right now the xarray plotting code (sometimes? always?) converts to np.ma.masked_array so we can't use this, yet. Additionally, this would be matplotlib specific, so once we add the plotting entrypoints we would need to find something new.

Which makes me wonder: should we introduce hooks based on the type of .data to prepare xarray objects for plotting? I think these hooks could be used to call cupy.Array.get, sparse.COO.todense or da.Array.compute before plotting, and because the argument to the hook would be a xarray object we could also have the hook for pint call .pint.dequantify. I'll open a new issue on the xarray issue tracker to see if I'm missing anything.

Copy link
Member

Choose a reason for hiding this comment

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

The issue with hard-coding is that we do expect more duck arrays in the future (e.g. pytorch and maybe uncertainties), and adding special cases for all of them would quickly become impossible.

I know, I'm not suggesting we special-case every combination forever, what I'm saying is that we don't always need to make something totally general right at the start. If we can make this work for pint and dask quickly I think we should, and worry about entrypoints when other duck-array libraries get to a similar point in their integration. We're not introducing much technical debt because this would be a small change for us and no external API change for the user (we will always want .plot() to work bare, however it is implemented behind the scenes).

I was hoping to make use of matplotlib.units for this.

This would be pretty nice. But again, I think we should make what we have work, then improve it later.

Which makes me wonder: should we introduce hooks based on the type of .data to prepare xarray objects for plotting?

I think this is a good idea for a long-term solution, which deserves it's own issue.

Copy link
Member

Choose a reason for hiding this comment

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

Which makes me wonder: should we introduce hooks based on the type of .data to prepare xarray objects for plotting? I think these hooks could be used to call cupy.Array.get, sparse.COO.todense or da.Array.compute before plotting,

This pretty much already happens in .values here

    data = data.get() if isinstance(data, cupy_array_type) else np.asarray(data)

it's just hardcoded instead of an entrypoint.

Copy link
Member

Choose a reason for hiding this comment

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

I've implemented this in pydata/xarray#5561

@keewis keewis closed this Feb 20, 2021
@keewis keewis reopened this Feb 20, 2021
@keewis keewis merged commit 7b03a3a into xarray-contrib:master Feb 20, 2021
@keewis keewis deleted the plotting-example branch February 20, 2021 12:51
@keewis keewis mentioned this pull request Feb 20, 2021
16 tasks
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