-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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 facet_col and animation_frame argument to imshow #2746
Conversation
@nicolaskruchten I think the PR is now ready for a first review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm excited for these additions!
@@ -186,6 +189,14 @@ def imshow( | |||
their lengths must match the lengths of the second and first dimensions of the | |||
img argument. They are auto-populated if the input is an xarray. | |||
|
|||
facet_col: int, optional (default None) | |||
axis number along which the image array is slices to create a facetted plot. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axis number along which the image array is slices to create a facetted plot. | |
axis along which the image array is sliced to create a facetted plot. |
I'm not entirely positive about my suggestion to remove 'number' in 'axis number'... From https://numpy.org/doc/stable/glossary.html, it looks like conventional terminology would be just 'axis' (as in 'axis 0' and 'axis 1'); I was tempted by 'axis index' but this would be confusing with dataframes (as in, 'index' vs 'columns'). Maybe 'axis number' is conventional terminology after all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
axis is fine indeed. It could also be "axis position" (as in the docstring of np.moveaxis). We can also ask other opinions, what do you think about the terminology @nicolaskruchten ?
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
Thanks for the useful comments @mkcor! |
…into imshow-animation
With the last batch of commits, it is now possible to use at the same time |
I've merged master with the recent image-related additions, there were a few conflicts for fix but all is good now! |
doc/python/imshow.md
Outdated
|
||
### Animations of xarray datasets | ||
|
||
*Introduced in plotly 4.11* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*Introduced in plotly 4.11* | |
*Introduced in plotly 4.13* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving up to the changes I suggested, because I can't wait to see this land in Plotly! ❤️
|
||
### Combining animations and facets | ||
|
||
It is possible to view 4-dimensional datasets (for example, 3-D images evolving with time) using a combination of `animation_frame` and `facet_col`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to view 4-dimensional datasets (for example, 3-D images evolving with time) using a combination of `animation_frame` and `facet_col`. | |
It is possible to view four-dimensional datasets (for example, 3-D images evolving with time) using a combination of `animation_frame` and `facet_col`. |
(because above text reads 'three-dimensional')
@@ -113,6 +117,19 @@ def imshow( | |||
their lengths must match the lengths of the second and first dimensions of the | |||
img argument. They are auto-populated if the input is an xarray. | |||
|
|||
animation_frame: int or str, optional (default None) | |||
axis number along which the image array is sliced to create an animation plot. | |||
If `img` is an xarray, `animation_frame` can be the name of one the dimensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If `img` is an xarray, `animation_frame` can be the name of one the dimensions. | |
If `img` is an xarray, `animation_frame` can be the name of one of the dimensions. |
|
||
facet_col: int or str, optional (default None) | ||
axis number along which the image array is sliced to create a facetted plot. | ||
If `img` is an xarray, `facet_col` can be the name of one the dimensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If `img` is an xarray, `facet_col` can be the name of one the dimensions. | |
If `img` is an xarray, `facet_col` can be the name of one of the dimensions. |
This PR will land, I promise! We just need plotly/plotly.js#5287 to be merged and released... In principle it'll be next week :) |
@@ -408,9 +499,18 @@ def imshow( | |||
else: | |||
raise ValueError( | |||
"px.imshow only accepts 2D single-channel, RGB or RGBA images. " | |||
"An image of shape %s was provided" % str(img.shape) | |||
"An image of shape %s was provided." | |||
"Alternatively, 3-D single or multichannel datasets can be" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3- or 4-D ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
# Now build figure | ||
col_labels = [] | ||
if facet_col is not None: | ||
slice_label = "slice" if labels.get("facet") is None else labels["facet"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PX default behaviour here is to have the label be called facet_col
and have it be overrideable via labels["facet_col"]
OR to have it be the value of facet_col
(i.e. "day of week") and have it be overrideable via labels["day of week"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OR to have it be the value of facet_col (i.e. "day of week") and have it be overrideable via labels["day of week"]
we can maybe leave this out for now/do it later... px.imshow
already doesn't do this for x/y/color and it mostly only applies in the case of xarray.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes exactly here we don't have column names, and the case of xarrays is handled correctly I think. So I can leave it like this?
) | ||
if animation_frame: | ||
fig.frames = frame_list | ||
fig.update_layout(layout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit odd to have layout
and layout_patch
here... it was odd before I guess but might be worth looking at merging them together earlier?
Thanks for the review! |
There are still a few |
OK, now that |
I added a changelog entry directly on master |
Work in progress in order to represent a 3d images as a series of facetted slices or as an animation
To do:
[x] Support xarrays and dimensions given by name instead of number
[x] Add some checks (what happens if facet_col is passed but the array is 2d etc) for various corner cases
[x] Add animation_frame argument following the same logic
[x] Write doc
[x] Make it possible to have at the same time
animate_frame
andfacet_col
so that it's possible to visualize 4d datasets.[ ] If possible, address in parallel plotly/plotly.js#3539 so that it's possible to have square pixels in subplots. Otherwise probably I'll just unmatch axes.
The PR should be rebased once #2691 is merged, for now it was branched off #2691's branch.