more fixes for v0.7/v1.0 #29
Conversation
This change attempts to make the adjoint method lazy, resolves a method ambiguity, restores the reinterpretc tests, and drops a couple of obsolete things in favor of v1.0
Codecov Report
@@ Coverage Diff @@
## master #29 +/- ##
=======================================
Coverage ? 100%
=======================================
Files ? 2
Lines ? 124
Branches ? 0
=======================================
Hits ? 124
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
|
I suspect the tests for adjoints are not really adequate, but am not sure what is wanted. |
|
Is |
src/ImageMetadata.jl
Outdated
| Base.reinterpret(::Type{T}, img::ImageMetaArray) where {T} = shareproperties(img, reinterpret(T, img.data)) | ||
| Base.reinterpret(::Type{T}, img::ImageMeta) where {T} = error("reinterpret method not defined for $(typeof(img)). Consider a MappedArray instead.") | ||
| function ImageCore.reinterpret(::Type{T}, img::ImageMeta{T2,N,A}) where {T,T2,N,A} | ||
| (A <: Array) || error("reinterpret method not defined for $(typeof(img)). Consider a MappedArray instead.") |
There was a problem hiding this comment.
I think the new reinterpret doesn't have this limitation anymore (?)
That said, I don't know this. This will need someone like Prof Holy to review (which I saw you already pinged, so I won't ping again)
There was a problem hiding this comment.
Correct, it's not a limitation anymore.
src/ImageMetadata.jl
Outdated
| Base.reinterpret(::Type{T}, img::ImageMetaArray) where {T} = shareproperties(img, reinterpret(T, img.data)) | ||
| Base.reinterpret(::Type{T}, img::ImageMeta) where {T} = error("reinterpret method not defined for $(typeof(img)). Consider a MappedArray instead.") | ||
| function ImageCore.reinterpret(::Type{T}, img::ImageMeta{T2,N,A}) where {T,T2,N,A} | ||
| (A <: Array) || error("reinterpret method not defined for $(typeof(img)). Consider a MappedArray instead.") |
There was a problem hiding this comment.
Correct, it's not a limitation anymore.
src/ImageMetadata.jl
Outdated
| @@ -184,8 +175,11 @@ end | |||
| Base.show(io::IO, img::ImageMeta) = showim(io, img) | |||
| Base.show(io::IO, ::MIME"text/plain", img::ImageMeta) = showim(io, img) | |||
|
|
|||
| Base.reinterpret(::Type{T}, img::ImageMetaArray) where {T} = shareproperties(img, reinterpret(T, img.data)) | |||
| Base.reinterpret(::Type{T}, img::ImageMeta) where {T} = error("reinterpret method not defined for $(typeof(img)). Consider a MappedArray instead.") | |||
| function ImageCore.reinterpret(::Type{T}, img::ImageMeta{T2,N,A}) where {T,T2,N,A} | |||
There was a problem hiding this comment.
reinterpret really belongs to Base, not ImageCore. (reinterpretc belongs to ImageCore)
|
Thanks! |
This change attempts to make the adjoint method lazy,
resolves a method ambiguity, restores the reinterpretc tests,
and drops a couple of obsolete things in favor of v1.0
Note: for background on reinterpret, see discussion in ImageCore PR