ENH: Added ability for the user to customize plot controls #94
Conversation
- User can select location of 1D cuts (horizontal can be top / bottom, vertical can be left / right) - User can add a title to the plot - User can add subtitles for horizontal and vertical axes - User can specify extent labels for image
|
@tacaswell, @danielballan, I'd love to hear your comments on this enhancement, which we worked on together with @kalebswartz7. Thanks! |
|
Code snippet to test functionality: img = np.loadtxt('res_int_pr_se.dat')
img = img.reshape((294, 528))
fig = plt.figure()
cs = CrossSection(fig, vert_loc='right', horiz_loc='bottom', title='Example', horiz_label='horizontal', vert_label='vertical', extent_labels=[-50, 50, -50, 50])
cs.update_image(img)
plt.show()Can replace / remove any of these parameters. Just need to read in a data file. |
There was a problem hiding this comment.
Looks good!
I tried with:
import numpy as np
from xray_vision.backend.mpl.cross_section_2d import CrossSection
import matplotlib.pyplot as plt
plt.ion()
img = np.random.random((10,20))
fig = plt.figure()
cs = CrossSection(fig, vert_loc='right', horiz_loc='bottom', title='Example', horiz_label='horizontal', vert_label='vertical', extent_labels=[-50, 50, -50, 50])
cs.update_image(img)
img = np.random.random((10,20))
fig = plt.figure()
cs = CrossSection(fig)
cs.update_image(img)I have a few comments. One main comment. I think the extent should be passed in update_img. I think this will simplify a lot of logic inside.
To get started, look for this line in code
self._init_artists(image, extent=extent)
and update things accordingly to get it working.
My feeling is that you will move all the extent logic into a self contained helper function and not have to worry about sharing or not sharing axes.
Also, right now, when I drag the image, the cross sections don't move. I think this will help with that.
If you decide to make a big change like this, i recommend naming this last commit to another branch so that you can easily go back if needed.
Anyway, thanks again for the contribution!
Also, in passing I noticed that the plots don't line up. I posted an issue here. Maybe you can try fixing that since while you're at it (I'm not sure if it is trivial or not though, I remember battling with these small details in the past quite a bit...)
| label that describes the x axis of the vertical plot | ||
| horiz_label : str, optional | ||
| label that describes the x axis of the horizontal plot | ||
| extent_labels : list, optional |
There was a problem hiding this comment.
a few comments:
- should be list of
strorint? (you typecast it later toint) - Why not just call it
extentto keep consistent with matplotlib:
extent : scalars (left, right, bottom, top), optional, default: None
The location, in data-coordinates, of the lower-left and upper-right corners. If None, the image is positioned such that the pixel centers fall on zero-based (row, column) indices.
-
I would at least recommend to remove "label" in variable name as it might make it sound like it can be any string.
These are just suggestions, feel free to disagree! :) -
Finally, I think this one might be a better API move: move this "extent" kwarg to the
update_imgmethod (i.e. we want to pass theextentto the function where we pass the image). This extent can potentially change with each new image, and so should come with the image. You could have it available in__init__as well but i think that might to too much of a support nightmare. Let me know what you think.
There was a problem hiding this comment.
-
Maksim and I discussed the
extentname, we just thought that if it wereextentonly there could be some confusion as the actual extent is not set - the image just appears as so. This is because when using actualextentthe image would not fill the entire subplot. I do agree to change the type from alisttoscalarswith a more fitting description. -
To go along with that we tried using the
self.init_artistsfunction to pass inextentbut we encountered the error described above. Ideallyextentwould actually be set but we couldn't figure out how to do so. If this were figured out theextent_labelsname could just simply beextentand it would make much more sense. -
I will be sure to check out moving the extent kwarg to the
update_imgfunction and look into aligning the plot lines - thanks for the feedback!
| horiz_label : str, optional | ||
| label that describes the x axis of the horizontal plot | ||
| extent_labels : list, optional | ||
| [x value initial, x value final, y value initial, y value final] |
There was a problem hiding this comment.
I would also call it "left, right, bottom top" since it's decsribing the image. But that's mainly because I'm used to matplotlib, so again more a suggestion!
There was a problem hiding this comment.
- This definitely makes sense!
| self.title = title | ||
| self.vert_label = vert_label | ||
| self.horiz_label = horiz_label | ||
| self.extent_labels = extent_labels |
There was a problem hiding this comment.
With my suggestion on extent, this will move to update_img and thus not need to be an instance attribute.
| int(self.extent_labels[1]), 5) | ||
| self.y_tick_values = np.linspace(int(extent_labels[2]), | ||
| int(extent_labels[3]), 5) | ||
| if vert_loc is None or vert_loc is not 'right': |
There was a problem hiding this comment.
i would move this lower and raise a ValueError:
if vert_loc is None:
vert_loc = left
if vert_loc not in {'right', 'left'}:
raise ValueError(f"Error, {vert_loc} not 'left' or 'right'")it's longer but I think it's more readable. Also, if the default value is not understood, its best to give the user feedback they've done something wrong (i.e. maybe they thought left=0 and right = 1 but not they're always getting left)
There was a problem hiding this comment.
- I will implement this change
| kwargs_vert = {'position': self.vert_loc, 'size': .5, 'pad': 0.1} | ||
| kwargs_horiz = {'position': self.horiz_loc, 'size': .5} | ||
| if self.extent_labels is None: | ||
| kwargs_vert['sharey'] = self._im_ax |
There was a problem hiding this comment.
why are you only sharing when there are extent labels?
I'm not very familiar, but I tried moving the image with the mouse without extent_labels and the cross section follows.
However, it does not for the image. Perhaps we should always share, and make sure the image extent is the same as the cross section's limits.
(more comments below suggesting what to do )
There was a problem hiding this comment.
- Initially there was no reason to get rid of the shared axes, but when they are shared the extent labels appear on the image rather than the vertical and horizontal cuts.
There was a problem hiding this comment.
Interesting. The code seems to be right since it applies the labels/ticks to the correct axes, self._ax_h and self._ax_v. Can it be a matplotlib bug? @tacaswell, thoughts?
| return "X: {x:d} Y: {y:d} I: {i:.2f}".format(x=col, y=row, i=z) | ||
| if self.extent_labels is not None: | ||
| return "X: {x:g} Y: {y:g} I: {i:g}".format( | ||
| x=col / im_shape[1] * (self.extent_labels[1] - |
There was a problem hiding this comment.
I think if you set the image extent you won't need this anymore
you can do this in (look for this line in code)
self._init_artists(image, extent=extent)
and update things accordingly to get it working.
| @@ -676,3 +774,12 @@ def autoscale_horizontal(self, enable): | |||
| @auto_redraw | |||
| def autoscale_vertical(self, enable): | |||
| self._ax_v.autoscale(enable=False) | |||
|
|
|||
|
|
|||
| def _swap_sides(key, enum): | |||
There was a problem hiding this comment.
can you add a comment explaining what this is doing and why?
There was a problem hiding this comment.
This is a helper function to easily swap the sides, e.g. 'left' -> 'right' and vice versa. In case the supplied value key is not in keys of enum, the default value from the enum is used (default is the one which has the True value).
@kalebswartz7, please create a docstring explaining this and the arguments to the function. Thanks!
vertical can be left / right)
closes #90