Conversation
hrobarts
left a comment
There was a problem hiding this comment.
Comments from code review meeting
|
@gfardell I think I've addressed all the comments from the code review meeting and also added a test with the simulated data |
gfardell
left a comment
There was a problem hiding this comment.
This will be a great tool.
My major comments are:
-
Backend selection and compatibility
- Consistency with FDK and CoR would be to accept a
backendparameter - I know it's only astra for now but could it be written in a way that would extend it to tigre for free once #2238 is in?
- A concern is that as ASTRA is GPL2, so we keep it as an optional import at runtime so the user is in control. This means code shouldn't silently depend on astra and it should be a user choice.
- Consistency with FDK and CoR would be to accept a
-
Documentation and doc strings
- Add a section in the documentation explaining the geometry and tested cases. Not everyone will have the same definition of the angle as "tilt" for example.
- Internal methods should start with
_and need docstrings and clear parameter names.
-
Memory use
- We need to think about reducing and reusing buffers where we can, both in terms of peak RAM and not reallocating memory each iteration where we could reuse it.
- For the acquisition and image data especially we need to be clear how many copies are used, this could be part of the documentation.
- We need to balance the cost of storing results with recomputing i.e. when we filter, or for the reference volume.
Overall it looks solid and well structured. Let me know when you need me to look again/talk it through.
I have not looked at tests yet.
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
| from scipy.spatial.transform import Rotation as R | ||
| from scipy.optimize import minimize | ||
| from scipy.ndimage import gaussian_filter, sobel |
There was a problem hiding this comment.
Is scipy a required or optional CIL dependency?
There was a problem hiding this comment.
It's in the run requirements but I'm not sure that's clear from the documentation
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
Wrappers/Python/cil/processors/LaminographyGeometryCorrector.py
Outdated
Show resolved
Hide resolved
| Convergence tolerance for optimisation of parameters, (tilt_tol_deg, CoR_tol_pix). | ||
| Defaults to (0.01, 0.01). |
There was a problem hiding this comment.
Do we require the geometry to have angles in degrees?
There was a problem hiding this comment.
No we update the rotation axis each time, we require the tilt to be defined in degrees to create the rotation matrix but it doesn't matter the initial units of the geometry
|
|
||
| return float(np.sum(r**2)) | ||
|
|
||
| def projection_reprojection(self, data, ig, ag, ag_ref, y_ref, tilt_deg, cor_pix): |
There was a problem hiding this comment.
How much memory is used by this processor? I see data, y_ref, yhat, r and recon here, but there's also various filtered versions. Are there savings to be found by reusing buffers and calculating in place?
There was a problem hiding this comment.
I think I've reduced some copies now, I think it could still be improved
Changes
Add automatic laminography alignment tool.
Projection matching method to optimise laminography reconstruction by searching tilt and centre of rotation offset
coarse_binningandfinal_binningangle_binningparameter_boundsstarting with an initial guessinitial_parameters.reduced_volumeTesting you performed
New demo script: TomographicImaging/CIL-Demos#280
Related issues/links
Checklist