-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add flux normaliser processor #1878
base: master
Are you sure you want to change the base?
Conversation
@gfardell we discussed whether the preview_configuration should just be an option in logging. At the moment I've left it as a function the users can access - what do you think? |
a910556
to
f8b513b
Compare
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 looking great and super useful! Excellent documentation in this PR to go along with it as well. as I have said in my comments, we need to make sure it lives somewhere useful!
I added some comments, mostly to improve the unit tests a bit and add some docs
Signed-off-by: Hannah Robarts <[email protected]>
…nto flux_normaliser
Hi @lauramurgatroyd thank you for your review! I have made lots of updates
I have added the notebook where I created the examples above to CIL-Demos/misc in this PR TomographicImaging/CIL-Demos#187 |
equal to the number of projections in the dataset. If flux=None, calculate | ||
flux from the roi, default is None. | ||
|
||
roi: dict (optional) |
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.
roi: dict (optional) | |
roi: dict (optional), default=None |
Please add defaults like this, as in docs guidance: https://tomographicimaging.github.io/CIL/nightly/developer_guide/
|
||
Parameters: | ||
----------- | ||
flux: float or list of floats (optional) |
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.
flux: float or list of floats (optional) | |
flux: float or list of floats (optional), default=None |
and/ or 'horizontal'. If an axis is not specified in the roi dictionary, | ||
the full range will be used, default is None. | ||
|
||
target: string or float |
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.
target: string or float | |
target: string or float, default='mean' |
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.
please list string options like: {'hi', 'ho'} in this example https://numpydoc.readthedocs.io/en/latest/example.html#example
if d in self.roi: | ||
# check indices are ints | ||
if not all(isinstance(i, int) for i in self.roi[d]): | ||
raise TypeError("roi values must be int, found {} and {}" | ||
.format(str(type(self.roi[d][0])), str(type(self.roi[d][1])))) | ||
# check indices are in range | ||
elif (self.roi[d][0] >= self.roi[d][1]) or (self.roi[d][0] < 0) or self.roi[d][1] > dataset.get_dimension_size(d): | ||
raise ValueError("roi values must be start > stop and between 0 and {}, found start={} and stop={} for direction '{}'" | ||
.format(str(dataset.get_dimension_size(d)), str(self.roi[d][0]), str(self.roi[d][1]), d )) | ||
# create slice |
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.
I wonder if we can make this some utility for validating roi throughout CIL
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.
(could make a separate issue)
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.
Hi Laura, this is a good point, I'll make an issue. I think the requirement for the ROI can vary between processors but there are definitely some common things.
|
||
return True | ||
# check if flux array contains 0s | ||
if 0 in self.flux: |
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.
does this catch 0.0
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, I've added some tests.
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.
Looks great and nice examples in CIL-Demos misc.
As discussed this needs:
- an example of 'first' in the demo
- the demo being merged to main
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 looking great. I'd be happy to merge it in as it is. But I think it would be quick to add a numba implementation of the actual calculation. We can look at it together this week.
roi: dict, optional | ||
Dictionary describing the region of interest containing the background | ||
in the image. The roi is specified as `{'axis_name1':(start,stop), | ||
'axis_name2':(start,stop)}`, where the key is the axis name 'vertical' | ||
and/ or 'horizontal'. If an axis is not specified in the roi dictionary, | ||
the full range will be used. |
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.
If this is just acquisition data then we know the axis names so I would remove the axis_name1
axis_name2
|
||
if 'horizontal' in dataset.dimension_labels: | ||
self.h_axis = dataset.get_dimension_axis('horizontal') | ||
self.h_size = dataset.get_dimension_size('horizontal') |
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.
I didn't know these methods existed, I'd always just found the index of dimension labels and then used that in shape!
angle: float, optional | ||
Angle to plot, default=None displays the data with the minimum | ||
and maximum pixel values in the roi, otherwise the angle to display | ||
can be specified as a float and the closest angle will be displayed. | ||
For 2D data, the roi is plotted on the sinogram. |
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.
Elsewhere I think we use 'angle' as the name of the axis, then the argument is the index of that projection in a list, not the value. It might be worth just discussing this further, not because it's wrong but it could just be a change of expected behaviour. Would it cause any issues if they specified an index?
It would also have to be careful about units - especially if we decide to convert things in the future and store in a defined way.
for i in range(num_proj): | ||
arr_proj = data.array.flat[i*proj_size:(i+1)*proj_size] | ||
if len(self.flux.flat) > 1: | ||
f = self.flux.flat[i] | ||
arr_proj *= self.target_value/f | ||
out.array.flat[i*proj_size:(i+1)*proj_size] = arr_proj |
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 would be trivial to add a numba implementation, and here it would speed things up a lot. So I think we should look at that before we merge.
Signed-off-by: Hannah Robarts <[email protected]>
Description
New FluxNormaliser processor to normalise data
Example Usage
Changes
Testing you performed
Demo scripts in https://github.com/TomographicImaging/CIL-Demos/blob/flux_normaliser_demo/misc/flux_normaliser.ipynb
Tests in test_DataProcessor.py TestFluxNormaliser class
Related issues/links
#1877
Checklist
Contribution Notes
Please read and adhere to the developer guide and local patterns and conventions.