Conversation
be62c09 to
0f557d5
Compare
0f557d5 to
e2aab93
Compare
e2aab93 to
23e6caf
Compare
1efde4a to
35746f5
Compare
35746f5 to
c334208
Compare
| fig, axes = plt.subplots(2, 3) | ||
|
|
||
| # interpolation='nearest' to reduce size of baseline image | ||
| axes[0, 0].imshow(x_1, interpolation='nearest', alpha=0.5) |
There was a problem hiding this comment.
are the other interpolations tested?
There was a problem hiding this comment.
Nope!,
I'm changing one of tests so that it is :)
There was a problem hiding this comment.
feeling silly but can't find the test with this change
There was a problem hiding this comment.
doesn't imshow usually default to nearest though? https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.imshow.html#matplotlib-axes-axes-imshow
Like what happens if interpolation is set to none?
ksunden
left a comment
There was a problem hiding this comment.
General thoughts on return types:
Doing things like float | tuple[float, ...] as is done for several things here (vmin/vmax, clip, etc) is potentially problematic.
Humans may easily work with that, but type checkers will likely yell that they didn't check for all possible outcomes
None is a bit of a special case in being more acceptable (easier to check, etc)
Consider moving these in new code to always return a tuple (even if single element) This keeps the branching needed to a minimum and is not too cumbersome to work for in the single variable case.
Obviously, existing APIs need to maintain back-compat, so this is limited to new code.
Consider whether conceptually an empty tuple is what is truly meant by the None case, but if it is not, retain None
We discussed change the behaviour of colorizer to always return tuples on the call last week. The relevant moving parts here are:
The Norm ABC must be typed as follows for backwards compatibility: For the Colorizer, I think it makes sense to force tuples on the getter, but allow both on the setter: For the _ColorizingInterface we have two options. def get_clim(self):
"""
Return the values (min, max) that are mapped to the colormap limits.
This function is not available for multivariate data.
"""
if self._colorizer.norm.n_components > 1:
raise AttributeError("`.get_clim()` is unavailable when using a colormap "
"with multiple components. Use "
"`.colorizer.get_clim()` instead.")
return self.colorizer.norm.vmin, self.colorizer.norm.vmaxOne reason why I favor option B, is that set_clim is already sufficiently complicated, because for scalar data it allows both signatures: |
d890452 to
b570ecc
Compare
b570ecc to
053e524
Compare
story645
left a comment
There was a problem hiding this comment.
Sorry for the very long delay in reviewing. Minor nits but I think this is fine otherwise.
| fig, axes = plt.subplots(2, 3) | ||
|
|
||
| # interpolation='nearest' to reduce size of baseline image | ||
| axes[0, 0].imshow(x_1, interpolation='nearest', alpha=0.5) |
There was a problem hiding this comment.
feeling silly but can't find the test with this change
Yeah I think since this is new and provisional, it's okay to encourage folks to use something else. But I would document the something else "for multivariate data use x,y,z" instead of just saying it's available. Also attn @timhoffm since this is an API question. |
Yes. Would it help for typing to differentiate 1d and Nd norm types? This could maybe help with overloading, so that we can more systematically separate the cases? But I'm not too much of a typing expert here and also have no oversight where this is used; e.g. it doesn't help if you store an arbitrary norm in an attribute and later query its limits. Maybe this approach would need to be extended to
Agree.
Yes, let's keep the |
2c433b5 to
553b1e1
Compare
553b1e1 to
99b9ed5
Compare
Comments regarding typing have been addressed
6f2dc67 to
ea43447
Compare
…how, pcolor, pcolormesh, and Collection
Co-authored-by: Kyle Sunden <git@ksunden.space>
ea43447 to
435d92a
Compare
|
@timhoffm @ksunden @story645 The next step in Bivariate and Multivariate Colormapping (view) will be to add equivalents to |
lib/matplotlib/axes/_axes.py
Outdated
| - (M, N): an image with scalar data. The values are mapped to | ||
| colors using normalization and a colormap. See parameters *norm*, | ||
| *cmap*, *vmin*, *vmax*. | ||
| - (v, M, N): if coupled with a cmap that supports v scalars |
There was a problem hiding this comment.
I probably should remember, but what is v the abbreviation for? - And since I don't: is ist important? It's just the number of components. Would K be a better name?
There was a problem hiding this comment.
Colormaps have an attribute n_variates, so "v" stands for variates.
But let us change it to K, as I think the logical jump to justify v is too long.
lib/matplotlib/tests/test_axes.py
Outdated
| fig, axes = plt.subplots(1, 6, figsize=(10, 2)) | ||
|
|
||
| axes[0].imshow((x_0, x_1), cmap='BiPeak', interpolation='nearest') | ||
| axes[1].matshow((x_0, x_1), cmap='BiPeak') |
There was a problem hiding this comment.
Do we want matshow to support multivar colors? Technically, that's possible, because it's a wrapper around imshow, but semantically it's currently defined as "Plot the values of a 2D matrix or array as color-coded image."
I haven't thought much about this, but it seems that the multivar extension is not helpful for matrix visualization. If that's the case, we should enforce in matshow that no multivar input is supported.
There was a problem hiding this comment.
My thinking was that matshow is not necessary to include in the first iteration. If we want to introduce it later we can make an issue. Not sure if it would be a "good first issue" or not, but it might yield the additional benefit of having someone else familiarize themselves with this side of the project.
There was a problem hiding this comment.
Agreed, a formal introduction to matshow needs additional consideration and should be investigated separately.
the question here is whether we implicitly allow multivariate mapping by doing nothing, or whether we explicitly check and prohibit multivariate mapping in matshow. I’m inclined towards the latter.
There was a problem hiding this comment.
matshow() starts with a line:
Z = np.asanyarray(Z)So since we can guarantee that the output is a np.array, we can easily check the size.
I added the following check.
Z = np.asanyarray(Z)
if Z.ndim != 2:
if Z.ndim != 3 or Z.shape[2] not in (1, 3, 4):
raise TypeError(f"Invalid shape {Z.shape} for image data")(This allows data of shape (M, N, 1), as I found that that is currently allowed on main).
There was a problem hiding this comment.
but it seems that the multivar extension is not helpful for matrix visualization
Someone could be visualizing index variables in a matrix same as they're visualizing index variables in a choropleth. I question if it's a good idea, but I think the general philosophy of this library should be to allow anything that isn't explicitly wrong.
|
@timhoffm I pushed the requested changes, thank you for the feedback :D |
timhoffm
left a comment
There was a problem hiding this comment.
Only two minor style issues.
This is fundamentally ready, but I think we cannot yet merge because we haven't branched 3.12 off yet (everything on main still targets 3.11).
This by itself should not go in without #30527 and potentially the other remaining topics in the project: https://github.com/orgs/matplotlib/projects/9/views/1.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
:D
Agreed |
Since there are a bunch of things that should go in together, do we want to experiment with a feature branch? I know we initially rejected the idea, but @QuLogic seems to be having some success with the font-overhaul branch. |
|
I'm -0.2 on feature branches. The advantage is that you can commit parts and then base other work on this without affecting the main branch. But this is also the disadvantage: If the feature changes code in areas that is also touched by the main branch, merging gets a nightmare quickly. The text overhaul branch works well because is in a very particular part of the code (and generally every change we make in that area is likely to change text rendering so should be on that branch). Multivar colormapping more broadly touches the code and is less suited for a feature branch. So far, we've been quite successful in incremental merges to main. It's just that we don't want to expose a multivar imshow as long as we don't have a suitable colorbar for that. And timing is accidentally so that the 3.11 release is between the readiness of the two parts. |

Exposes the functionality of
MultiNorm,BivarColormapandMultivarColormapto the top level plotting functionsax.imshow(),ax.pcolor()andax.pcolormesh(). This closes #30526, see Bivariate and Multivariate ColormappingAs a side-effect of the pcolor/pcolormesh implementation,
Collectionalso gets the new functionality.In short, this PR allows you to plot multivariate data more easily, but it does not:
These will come in later PRs. See Bivariate and Multivariate Colormapping
Examples demonstrating new functionality: