Support units #103
Support units #103
Conversation
Codecov Report
@@ Coverage Diff @@
## master #103 +/- ##
=======================================
Coverage 99.81% 99.81%
=======================================
Files 44 44
Lines 2172 2181 +9
=======================================
+ Hits 2168 2177 +9
Misses 4 4
Continue to review full report at Codecov.
|
src/fbp/image_geom.jl
Outdated
| throw(DimensionMismatch("mask size $(size(mask)) vs dims $dims")) | ||
| new{D}(dims, Float32.(deltas), Float32.(offsets), mask) | ||
| end | ||
| struct ImageGeom{D, S <: NTuple{D,Number}} |
There was a problem hiding this comment.
@johnnychen94 If you have a chance to skim these changes I would be interested in your feedback.
The deltas here are pixel sizes and originally I made them all Float32 but recently I've been interested in trying to use physical units more via Unitful, so I've relaxed the type to Number. (Unfortunately that also allows complex numbers but so it goes.) I know I could use a Union of Real and Unitful.Quantity but I'd rather not add Unitful as a dependency because maybe there are/will be other units packages and I prefer to keep it general.
BTW, for a video the we might have units of (m, m, s) for 2D+time so I want to allow deltas to be a general Tuple of numbers.
Anyway, one specific question for you is whether you think that including the type S of the Tuple as part of this struct parameter is useful or not. I definitely can dispatch on the dimension D but I'm not sure that I will dispatch on the type of the deltas Tuple. Any thoughts on this or anything else here?
I plan on making more such changes elsewhere in the package to support units so now is the best time to prod me in a good direction :)
There was a problem hiding this comment.
Looks like ImageGeom still uses the old object-oriented design, is it possible to instead embrace the array interface here?
I don't know the usage of mask here, but it seems that the other fields dims, deltas, offsets can be replaced by OffsetArrays.
For unitful arrays, perhaps AxisArrays.jl is a handful concept (although I never tried it)
There was a problem hiding this comment.
Thanks for the comments. The offsets here are non-integer so OffsetArrays is inapplicable. But AxisArrays looks useful!
I realize now that I need to add documentation to explain what this type does. So I might get back to you with another question after I do that. (This is not an array type; it is a description of the pixel geometry for image reconstruction.) More later - thanks!
There was a problem hiding this comment.
it is a description of the pixel geometry for image reconstruction
This isn't a very distinctive definition to me. I see array properties like ndim, zeros, ones, but that could be replaced by ndims(im), zeros(size(im)), ones(size(im)). How much of it will be left if we 1) separate it into a lazy array 2) remove unnecessary wrapper (e.g., im.plot, im.shape)?
Maybe we can start by deprecating the old usage in favor of the functional julian way? That could possibly reduce some complexities and simplify the code design.
There was a problem hiding this comment.
I have a lot of legacy code that uses this "matlab" struct style and properties so it will be a while before I am ready to start deprecating. But I have added documentation about ImageGeom in the latest push so hopefully that will deploy and clarify. I still have a lot of other code that I want to upgrade to support units and that will be a higher priority for a while. But thanks for steering me towards the Julia way :)
There was a problem hiding this comment.
@johnnychen94 fyi i took your comments to heart and created a new "functional" version here:
https://github.com/JuliaImageRecon/ImageGeoms.jl
See the docs (via Literate) for an explanation.
Eventually I will deprecate a lot here.
|
I am merging in hopes that it will trigger the docs deploy, but not tagging yet because more changes planned. |
The goal here is to relax
RealtoNumberfor quantities like image pixel sizes and circle radii to support Unitful.