Orientational sites #307
Orientational sites #307
Conversation
There was a problem hiding this comment.
Just some comments on this PR. I'm not entirely sure why we need to have a subclass of Volume for what is essentially peakfinding a 2D histogram. It's a lot to go through to understand what is going on, especially because in the plots, the code also goes through the OrientationalVolume. Conceptually, I think it would be easier to understand this if all the methods were self-contained on Orientations. This is mostly about rearranging bits, other than that it looks pretty good 😅
src/gemdat/volume.py
Outdated
| class OrientationalVolume(Volume): | ||
| """Container for orientational volume data, that are in spherical | ||
| coordinates.""" | ||
| shape: tuple[int, int] | ||
| radii: np.ndarray |
There was a problem hiding this comment.
I'm not convinced that subclassing Volume makes sense here. The other methods on Volume will not work with the data being passed for the OrientationalVolume.
| theta, phi, values = self.data.T | ||
|
|
||
| x = theta | ||
| y = phi |
There was a problem hiding this comment.
| theta, phi, values = self.data.T | |
| x = theta | |
| y = phi | |
| theta, phi, values = self.data.T |
src/gemdat/volume.py
Outdated
| peaks = [(y[int(i), int(j)], x[int(i), int(j)]) | ||
| for i, j, sigma in blobs] |
There was a problem hiding this comment.
| peaks = [(y[int(i), int(j)], x[int(i), int(j)]) | |
| for i, j, sigma in blobs] | |
| peaks = [(phi[int(i), int(j)], theta[int(i), int(j)]) | |
| for i, j, _ in blobs] |
| ov = orientations.to_volume(shape=shape, normalize_area=normalize_histo) | ||
| theta, phi, values = ov.data.T |
There was a problem hiding this comment.
Can we make this something like:
| ov = orientations.to_volume(shape=shape, normalize_area=normalize_histo) | |
| theta, phi, values = ov.data.T | |
| theta, phi, values = orientations.histogram(normalize=True) |
The histogram can be re-used to get the peaks.
| self.shape = shape | ||
| self.radii = radii | ||
|
|
||
| def orientational_peaks( |
There was a problem hiding this comment.
We already have find_peaks on Volume, this extra method could be a bit confusing. If going down this route, have you considered overwriting the original method?
| def orientational_peaks( | |
| def find_peaks( |
| shape: tuple[int, int] | ||
| radii: np.ndarray |
There was a problem hiding this comment.
Radii and shape are not used, I'd recommend removing them from this class.
| shape: tuple[int, int] | |
| radii: np.ndarray |
|
I fixed all the merge conflicts with main in this PR |
This PR uses blob_dog to identify the orientations which are explored more often.
Now running
plot_rectilinear(add_peaks=True)you can obtain figures like this one:The calculation of the peaks has been implemented by introducing an
OrientationalVolumeclass, inherited fromVolume.This PR may be redundant once #306 is implemented