Repository navigation
Add marginals_plot - #618
Conversation
| from ..graphics import imagefigure, linefigure | ||
|
|
||
|
|
||
| def marginals_plot( |
There was a problem hiding this comment.
Do we need the _plot suffix? This being a plotting library, maybe it's clear enough with plopp.marginals?
| "outputs": [], | ||
| "source": [ | ||
| "da = pp.data.data2d()\n", | ||
| "pp.marginals_plot(da)" |
There was a problem hiding this comment.
I tried it with bin edges (da = pp.data.data2d(binedges=True)) and it fails with
DimensionError: Cannot add coord 'y' of dims (y: 40) to DataArray with dims (y: 41)
There was a problem hiding this comment.
Now that I think of it, bin edges might be an issue for the side plot... 😬
There was a problem hiding this comment.
I don't see a good solution to this except implementing support for orientation='vertical' in Plopp. I could try this. Or we just raise an error, saying that bin edges are not supported along the vertical dim.
There was a problem hiding this comment.
Just raise for now, I can try to fix later. Can you make an issue so I don't forget?
|
|
||
| :::{grid-item-card} Marginals plot | ||
| :link: marginals-plot.ipynb | ||
| :img-bottom: ../_static/plotting/marginals-plot.png |
There was a problem hiding this comment.
I think you also need to add it to the top-level index (currently does not show up there).
| cax = divider.append_axes("right", "4%", pad="5%") | ||
|
|
||
| main_data_node = data if isinstance(data, Node) else Node(data) | ||
| fig = imagefigure(main_data_node, ax=ax_2d, cax=cax, cbar=True, **plot_kwargs) |
There was a problem hiding this comment.
If I pass the cbar=False arg if I want to hide it, it clashes with the hard-coded one here.
| np.testing.assert_allclose(top_line.get_xdata(), da.coords[xdim].values) | ||
| np.testing.assert_allclose(top_line.get_ydata(), da.sum(ydim).values) | ||
| np.testing.assert_allclose(side_line.get_xdata(), da.sum(xdim).values) | ||
| np.testing.assert_allclose(side_line.get_ydata(), da.coords[ydim].values) |
There was a problem hiding this comment.
Ideally, we should avoid having matplotlib specifics in these tests (such as get_xdata()).
Instead, we should grab the Plopp artists and verify the values they hold agree with what we expect
However, I don't see a nice way of doing this, as we only return the 2d figure, which is probably the right thing to do. So we can keep it as it is (I know you've said before that maybe we should just commit fully to matplotlib...)
There was a problem hiding this comment.
TBH, the tests were generated by Claude. So I rely on you to tell me if they should be changed.
That said, I don't think marginals can work with anything except matplotlib because it uses make_axes_locatable directly.
2f4d8cf to
eadc040
Compare
| cax = divider.append_axes("right", "4%", pad="5%") if cbar else None | ||
|
|
||
| main_data_node = data if isinstance(data, Node) else Node(data) | ||
| fig = imagefigure(main_data_node, ax=ax_2d, cax=cax, cbar=cbar, **plot_kwargs) |
There was a problem hiding this comment.
If you're tired of trying to get this working for everything, I can take over.
I just realised that the plot won't work for inputs that are missing coords (and probably other things).
We can either just use plot instead of imagefigure and linefigure as they also accept nodes as inputs?
There was a problem hiding this comment.
I worked around one issue when building the side data. But plot fails because the coord is unsorted. How would you deal with that? (Feel free to push a change if that is easier)
There was a problem hiding this comment.
I pushed an update with a flag to suppress the unsorted coord warning.
Now it also works with raw numpy arrays as inputs.
It works and looks good to me. |
This is scipp/ess#859 but cleaned up a little and with support for interactive figures.