Thankfully these phase_contrast changes are as easy as pie #577
Conversation
gvarnavi
changed the title
| if not positions_mask[rx, ry]: | ||
| continue |
There was a problem hiding this comment.
Is this skipping posistions where posistions_mask[rx,ry] == False if so is the following correct and more readable"
if posistion_mask[rx,ry] is False:
continue Is it worth splitting this into two for loops so that posistion_mask is Not None, is checked once, and not per probe position?
There was a problem hiding this comment.
Correct, this is skipping positions where the mask is false. Not sure I understand the for loop suggestion, this has to be checked per probe position.
Re: style - PEP8 explicitly says comparing boolean values to True/False with == or is is bad form.
There was a problem hiding this comment.
Good catch. One day I'll learn to not try and refactor numpy Boolean masks
There was a problem hiding this comment.
About the for loop, this checks if posistion_mask is not None at every probe position. Would it be better to do ~:
if positions_mask is None:
intensities = get_shifted_ar(
diffraction_intensities[rx, ry],
-com_fitted_x[rx, ry],
-com_fitted_y[rx, ry],
bilinear=True,
device="cpu",
)
if crop_patterns:
intensities = intensities[crop_mask].reshape(
region_of_interest_shape
)
mean_intensity += np.sum(intensities)
amplitudes[counter] = np.sqrt(np.maximum(intensities, 0))
counter += 1
else:
for rx in range(diffraction_intensities.shape[0]):
for ry in range(diffraction_intensities.shape[1]):
if not positions_mask[rx, ry]:
continue
intensities = get_shifted_ar(
diffraction_intensities[rx, ry],
-com_fitted_x[rx, ry],
-com_fitted_y[rx, ry],
bilinear=True,
device="cpu",
)
if crop_patterns:
intensities = intensities[crop_mask].reshape(
region_of_interest_shape
)
mean_intensity += np.sum(intensities)
amplitudes[counter] = np.sqrt(np.maximum(intensities, 0))
counter += 1
| def show_fourier_probe( | ||
| self, | ||
| probe=None, | ||
| remove_initial_probe_aberrations=False, | ||
| cbar=True, | ||
| scalebar=True, | ||
| pixelsize=None, |
There was a problem hiding this comment.
kind of trivial, but it would be nice if these were type hinted
| figsize = kwargs.pop("figsize", (6, 6)) | ||
| chroma_boost = kwargs.pop("chroma_boost", 2) | ||
| chroma_boost = kwargs.pop("chroma_boost", 1) |
There was a problem hiding this comment.
this is cool I didn't know you could pop from a dict
| plot_fourier_probe: bool, optional | ||
| If true, the reconstructed complex Fourier probe is displayed | ||
| remove_initial_probe_aberrations: bool, optional | ||
| If true, when plotting fourier probe, removes initial probe | ||
| to visualize changes | ||
| padding : int, optional | ||
| Pixels to pad by post rotating-cropping object |
There was a problem hiding this comment.
I don't think these are optional as there's no deafult value. If wanted to be treated as optional, it should be remove_initial_probe_aberrations: Optional[bool] = None, or remove_initial_probe_aberrations: bool | None = None (this needs from __future__ import annotations as the first import)
| chroma_boost = kwargs.pop("chroma_boost", 2) | ||
| else: | ||
| chroma_boost = kwargs.pop("chroma_boost", 1) | ||
| chroma_boost = kwargs.pop("chroma_boost", 1) |
There was a problem hiding this comment.
is removing default for chroma_boost = 2 a preference thing?
| probe = list( | ||
| asnumpy( | ||
| self._return_fourier_probe( | ||
| probe, | ||
| remove_initial_probe_aberrations=remove_initial_probe_aberrations, | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
trivial, but I think literal [] is prefered to list
| @@ -3052,6 +3092,7 @@ def show_fourier_probe( | |||
| def show_transmitted_probe( | |||
| self, | |||
| plot_fourier_probe: bool = False, | |||
| remove_initial_probe_aberrations=False, | |||
There was a problem hiding this comment.
needs docstring added
| if remove_initial_probe_aberrations: | ||
| probe_array = self.probe_fourier_residual | ||
| else: |
There was a problem hiding this comment.
same self.probe_fourier_residual[0] comment
| remove_initial_probe_aberrations: bool, optional | ||
| If true, when plotting fourier probe, removes initial probe |
| @@ -2158,6 +2165,7 @@ def visualize( | |||
| plot_convergence: bool = True, | |||
| if not positions_mask[rx, ry]: | ||
| continue |
There was a problem hiding this comment.
Good catch. One day I'll learn to not try and refactor numpy Boolean masks
| if not positions_mask[rx, ry]: | ||
| continue |
There was a problem hiding this comment.
About the for loop, this checks if posistion_mask is not None at every probe position. Would it be better to do ~:
if positions_mask is None:
intensities = get_shifted_ar(
diffraction_intensities[rx, ry],
-com_fitted_x[rx, ry],
-com_fitted_y[rx, ry],
bilinear=True,
device="cpu",
)
if crop_patterns:
intensities = intensities[crop_mask].reshape(
region_of_interest_shape
)
mean_intensity += np.sum(intensities)
amplitudes[counter] = np.sqrt(np.maximum(intensities, 0))
counter += 1
else:
for rx in range(diffraction_intensities.shape[0]):
for ry in range(diffraction_intensities.shape[1]):
if not positions_mask[rx, ry]:
continue
intensities = get_shifted_ar(
diffraction_intensities[rx, ry],
-com_fitted_x[rx, ry],
-com_fitted_y[rx, ry],
bilinear=True,
device="cpu",
)
if crop_patterns:
intensities = intensities[crop_mask].reshape(
region_of_interest_shape
)
mean_intensity += np.sum(intensities)
amplitudes[counter] = np.sqrt(np.maximum(intensities, 0))
counter += 1
|
The decision was to accept these with the formatting issues, and we will address them in a subsequent PR. |
Thankfully these phase_contrast changes are as easy as pie Former-commit-id: 3397349
Various small fixes and features to merge before MRS: