-
Notifications
You must be signed in to change notification settings - Fork 60
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
ROI edits #272
ROI edits #272
Conversation
image_shape : tuple | ||
2 element tuple defining the number of pixels in the detector. | ||
Order is (num_rows, num_columns) | ||
shape : rr, cc : tuple |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not valid numpdoc, the (rr, cc) should go in the description.
|
||
# indices of rings | ||
labels_grid = np.digitize(np.ravel(grid_values), np.array(edges), | ||
edges = np.atleast_2d(np.asarray(edges)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well just ravel it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bump on this, I think it simplifies the rest of the code.
To divide the rings into pies and removed the divide_cirecle functon
More tests
if slicing_is_list: | ||
slicing = np.asarray(slicing) + offset_angle | ||
else: | ||
slicing = np.linspace(0, 360, slicing) + offset_angle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
radian conversion issue
|
||
edges = roi.ring_edges(first_q, width=delta_q, spacing=4, | ||
num_rings=num_rings) | ||
label_array = roi.segmented_rings(edges, slicing, calibrated_center, img_dim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least test that the histogram comes back correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sameera2004 Can you do this?
|
||
Returns | ||
------- | ||
phi : array | ||
:math:`\\phi`, the angle from the vertical axis. | ||
:math:`\\phi \\el [-\pi, \pi]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you leave the bit about the range and where the angle is measured from in?
…id instead of core.pixel_to_phi and core.pixel_to_radius
…ation_rings to get the correct label_aray
…tests. Will add them near future
label_mask, pixel_list = corr.extract_label_indices(label_array) | ||
# number of pixels per ROI | ||
num_pixels = np.bincount(label_mask, minlength=(np.max(label_mask)+1)) | ||
num_pixels = num_pixels |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sameera2004 What was your intention here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To add more tests, If you want you can take that out, and we can merge this PR. I will work on adding more tests to these functions
calibrated_center[0])) | ||
return np.arctan2(X, Y) | ||
center[0])) | ||
return np.arctan2(y, x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This got flipped, did I have it wrong before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is an intentional change in behavior, documented and tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...and now also noted specifically in the API changes docs.
@@ -0,0 +1,8 @@ | |||
Grid-building functions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make this a rst file?
ENH/API : overhaul ROI generation API
Some tweaks to the roi.py module:
roi.rings
. This is the only ring-related function that should need to know the image shape or calibrated center. The input to this function is the most general way to specify rings: their inner and outer radii.This also addresses some name changes suggested in #267, which was superceded by #268 and this PR.