2550 wedge slicer #2566
2550 wedge slicer #2566
Conversation
This fixes the error due to the confusion between base and base.base which had all interactors redraw the plot first and updating from the move second. This was resolved in the "moveend" method but made using the mouse to set the interactor rather difficult to do precisely. This was also cleanded up in the three interactor/slicer modules not currently used but that probably will be for the needs of the magnetic/crystal scattering communities. Duplicate draws were removed when appropriate.
First pass at fixing the slicer classes docstrings.
There are three unused slicer modules that are set up for wedge type slicers. These were never implemented. I suspect that in fact it was orinally planed to use this for sector averaging where perhaps the wedge became a sector by making r1=0 and r2 = qmax. However as it stands it left two slicers classes called SectorInteractor. This unused class is renamed to the more appropriate WedgeInteractor in preparation for being used for crystal type data.
…'left' and 'right' are set correctly when calling SideInteractor.update().
…Also added back a superfluous self.update() call for consistency with the other slicers.
…r implementation.
… new wedge slicer.
… will not spawn yet however.
…r sets them for better consistency with the other interactor elements.
…w, fixed slicer plotting bugs, and corrected the nbins and npts implementations.
…teractor to draw line from origin instead of across the whole plot.
|
Following some further testing, I've discovered a bug in the |
…o corrected the angles being sent to manipulations.py
butlerpd
added
the
Discuss At The Call
| """ | ||
| def __init__(self, base, axes, color='black', zorder=5, r=1.0, | ||
| theta=np.pi / 3, phi=np.pi / 8): | ||
| BaseInteractor.__init__(self, base, axes, color=color) |
There was a problem hiding this comment.
super().__init__(base, axes, color=color) is perhaps more appropriate here
There was a problem hiding this comment.
Yep, I can see the sense in that. All of the slicer interactors call BaseInteractor.__init__() however, would you advise making this change universally or just in WedgeInteractor, ArcInteractor and RadiusInteractor?
|
I started looking at the functionality. Loaded Granted, I was being silly and started the slicer on the residuals plot, without the |
There was a problem hiding this comment.
Good code, just some minor comments not influencing the overall picture.
Great job of adding and correcting lots of comments in the code!
The only issue is the runtime exception when applying the slicer onto a residual plot, but this applies to all slicers, so needs to be made a separate GH issue.
| label='pick', zorder=zorder, | ||
| visible=True)[0] | ||
| # Define the arc | ||
| [self.arc] = self.axes.plot([], [], linestyle='-', marker='', color=self.color) |
There was a problem hiding this comment.
just a small lack of consistency
a = method()[0] vs [a] = method()
Both unpack the first plot so please choose one way of doing it
| self.arc.remove() | ||
| except: | ||
| # Old version of matplotlib | ||
| for item in range(len(self.axes.lines)): |
There was a problem hiding this comment.
should we be worrying about this "old version" path anymore? remove() is available in >3.0, which we have standardized on for quite some time now.
| yval = 1.0 * self.radius * np.sin(angleval) | ||
| x.append(xval) | ||
| y.append(yval) | ||
|
|
There was a problem hiding this comment.
this can be improved (performance) by taking out the factor and offset calculations out of the loop.
precalculate
angle_factor = 2 * self.phi / (self.npts - 1)
angle_offset = self.theta - self.phi
and just use those in the loop, e.g. with
x = [self.radius * np.cos(angle_factor * i + angle_offset) for i in range(self.npts)]
y = [self.radius * np.sin(angle_factor * i + angle_offset) for i in range(self.npts)]
| MIN_PHI_DIFFERENCE = 0.01 | ||
| isValid = True | ||
|
|
||
| # Stitched together from other slicers. There could be a neater way. |
There was a problem hiding this comment.
You could probably invent something fancy here but maybe readability is better?
"""
Validate input from user.
Values get checked at apply time.
"""
def check_radius_difference(param_name, other_radius_name, param_value):
if np.fabs(param_value - self.getParams()[other_radius_name]) < self.dqmin:
return f"Inner and outer radii too close. Please adjust."
elif param_value > self.qmax:
return f"{param_name.capitalize()} exceeds maximum range. Please adjust."
return None
def check_phi_difference(param_value):
if np.fabs(param_value) < 0.01:
return "Sector angles too close. Please adjust."
return None
def check_bins(param_value):
if param_value < 1:
return "Number of bins cannot be <= 0. Please adjust."
return None
validators = {
'r_min': lambda value: check_radius_difference('r_min', 'r_max', value),
'r_max': lambda value: check_radius_difference('r_max', 'r_min', value),
'delta_phi [deg]': check_phi_difference,
'nbins': check_bins
}
if param_name in validators:
error_message = validators[param_name](param_value)
if error_message:
print(error_message)
return False
return True
Description
This PR addresses issue #2550, implementing the Wedge Slicer. The desired functionality is present, and the ideas behind
RadiusInteractor.pycould be applied to simplifySectorSlicer.pyif the slicers are ever refactored. Also included are the cherry-picked commits from branch2482-problem-with-dragging-slicer-elements-on-plot, which served as a base for my work here.Closes #2550 and also closes #2482
How Has This Been Tested?
The slicer interactor elements were adjusted in various sequences, through both the GUI and the Edit Slicer Parameters window accessible via the right-click menu. It was found that these parameters behaved consistently, and each of the averaging directions produced convincing-looking 1D plots.
Whether the 1D plot results produced by the slicer are accurate depends on whether the classes
SectorQandSectorPhifromsasdata/sasdata/data_util/manipulations.pyare working as intended. They should be sufficient for high resolution data, but there may be further work necessary to improve their reliability with lower resolution data.Review Checklist (please remove items if they don't apply):