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 dimension slicer plot #19

Merged
merged 18 commits into from
Sep 19, 2022
Merged

Add dimension slicer plot #19

merged 18 commits into from
Sep 19, 2022

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Sep 16, 2022

This allows to slice higher dimensions with sliders.

Example:

%matplotlib widget
import plopp as pp

da = pp.data.dense_data_array(ndim=3)
pp.slicer(da, keep=['z', 'y'])  # slice out dimension 'x' -> makes a 2d image with 1 slider
pp.slicer(da, keep=['z'])  # slice out dimensions 'x' and 'y' -> makes a 1d line with 2 sliders

We are not adding the buttons from the old plotting that allowed to flip the dimensions interactively because this would lead to too much entangled code between the Mesh and the Figure, where a new Mesh would have to be made, the axis limits would have to be changed, and what to do with the log scales of the axes.

Instead, we make an easy to use API where keep is a list of dimensions to allow the user to quickly get what they want by re-plotting, instead of changing dims dynamically after the plot was made.

Fixes #14

@nvaytet nvaytet marked this pull request as draft September 16, 2022 19:36
@nvaytet nvaytet marked this pull request as ready for review September 17, 2022 11:06
@nvaytet nvaytet marked this pull request as draft September 18, 2022 07:14
@SimonHeybrock
Copy link
Member

pp.slicer(da, dims=['x'])  # slice out dimension 'x' -> makes a 2d image with 1 slider
pp.slicer(da, dims=['x', 'y'])  # slice out dimensions 'x' and 'y' -> makes a 1d line with 2 sliders

Comment before having looked at the implementation: I feel this is backwards. Wouldn't it be more natural to specify the dims the user wants to see?

@SimonHeybrock
Copy link
Member

We are not adding the buttons from the old plotting that allowed to flip the dimensions interactively because this would lead to too much entangled code between the Mesh and the Figure, where a new Mesh would have to be made, the axis limits would have to be changed, and what to do with the log scales of the axes.

Would it be an option to make one plot for each axis combination, and show only the selected one? The old buttons next to the slider essentially act like tab-selection buttons, i.e., it simply selects a different figure to show.

@nvaytet
Copy link
Member Author

nvaytet commented Sep 19, 2022

Comment before having looked at the implementation: I feel this is backwards. Wouldn't it be more natural to specify the dims the user wants to see?

I don't mind either way.
Maybe we should call the argument keep like in sc.collapse?

pp.slicer(da, keep=['x'])  # keep only 'x' -> makes a 1d line with 2 sliders
pp.slicer(da, keep=['x', 'y'])  # keep 'x' and 'y' -> makes a 2d image with 1 slider along 'z'

@nvaytet
Copy link
Member Author

nvaytet commented Sep 19, 2022

Would it be an option to make one plot for each axis combination, and show only the selected one? The old buttons next to the slider essentially act like tab-selection buttons, i.e., it simply selects a different figure to show.

I feel this is really adding complication for not much gain. And the different tabs wouldn't help you with the handling of the log scales on the axes.
I really don't think the buttons will be used much, if it's easy for the user to specify when making the plot what they want to keep/slice.

In any case, I don't want to add them now and make things more complicated than they are. If users later say this is an absolute must have, then we can add them later, maybe using what you suggest.

@SimonHeybrock
Copy link
Member

Comment before having looked at the implementation: I feel this is backwards. Wouldn't it be more natural to specify the dims the user wants to see?

I don't mind either way. Maybe we should call the argument keep like in sc.collapse?

pp.slicer(da, keep=['x'])  # keep only 'x' -> makes a 1d line with 2 sliders
pp.slicer(da, keep=['x', 'y'])  # keep 'x' and 'y' -> makes a 2d image with 1 slider along 'z'

We should probably just get user feedback/opinions.

@nvaytet nvaytet marked this pull request as ready for review September 19, 2022 07:47
"id": "a175931f-a0fd-48f7-83cc-9db14d454535",
"metadata": {},
"source": [
"# Slicer plot\n",
Copy link
Member

Choose a reason for hiding this comment

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

As shown by a user comment on Slack, the name "Slicer" may be misleading?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any better ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Ask the users?

Copy link
Member Author

Choose a reason for hiding this comment

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

Greg says slicer is fine in the end.

@@ -13,6 +13,7 @@ testpaths = "tests"
filterwarnings = [
"error",
"ignore::UserWarning",
"ignore::PendingDeprecationWarning",
Copy link
Member

Choose a reason for hiding this comment

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

Really?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how to make this specific to Matplotlib. This arose when updating to Matplotlib 3.6 where get_cmap raises this warning. They say to use the syntax

cmap = matplotlib.colormaps[name]

instead of

cmap = matplotlib.cm.get_cmap(name)

However, the new syntax does not exist in mpl<3.5. I thought I would keep it compatible?
Alternatively I can just use the new syntax and put a lower bound on the MPL requirement?
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Leave it for now.

Comment on lines 137 to 141
The object to be plotted. Possible inputs are:
- Variable
- Dataset
- DataArray
- numpy ndarray
Copy link
Member

Choose a reason for hiding this comment

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

Type hints should take care of this?

@nvaytet
Copy link
Member Author

nvaytet commented Sep 19, 2022

Merging so I can more easily move with other PRs

@nvaytet nvaytet merged commit c65017a into main Sep 19, 2022
@nvaytet nvaytet deleted the dimension_slicer branch September 19, 2022 13:21
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.

Add slicer plot for slicing higher dimensions
2 participants