fixed select_region and colleagues. #13
Conversation
… argument to select_region_view
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 91.28% 91.74% +0.45%
==========================================
Files 10 11 +1
Lines 218 218
==========================================
+ Hits 199 200 +1
+ Misses 19 18 -1 ☔ View full report in Codecov by Sentry. |
src/selection_tools.jl
Outdated
| @@ -311,9 +318,9 @@ julia> dst=select_region(a,new_size=(10,10), dst_center=(1,1)) # pad a with zero | |||
| 0.0 0.0 0.0 0.0 0.0 0.0 2.0 2.0 2.0 2.0 | |||
| ``` | |||
| """ | |||
| function select_region!(src, dst; new_size=size(dst), | |||
| function select_region!(src, dst; new_size=ntuple((d)->typemax(Int)÷2,Val(length(size(src)))), | |||
There was a problem hiding this comment.
why typemax?
You can also use nothing to handle that.
Also instead of using length(size(src)) better do src::AbstractArray{T, N} and use the parametric type N.
There was a problem hiding this comment.
I wanted to avoid branches in the code. First had a "nothing" version but this would anyway then branch to the same expression.
typemax is needed as the index-calculation which is used explicitely considers a finite copy-size. If the user specifies center=-100, dst_center=-100 it should still work. For copy operations such as in select_region!() the code still insists on the centers, if the new_size is specifies, but it should allow for maximal src and dst overlap, in case the user does not specify new_size.
There was a problem hiding this comment.
... the use of length was fixed now.
src/selection_tools.jl
Outdated
| function select_region!(src, dst; new_size=size(dst), | ||
| center=size(src).÷2 .+1, dst_center=size(dst).÷ 2 .+1, operator! =assign_to!) | ||
| new_size = Tuple(expand_size(new_size, size(dst))) | ||
| function select_region!(src::AbstractArray{T, N}, dst; new_size=ntuple((d)->typemax(Int)÷2,Val(N)), |
There was a problem hiding this comment.
still think that typemax is hacky.
even -1 would be better
There was a problem hiding this comment.
Do you prefer this version? I hope it is correct for even and odd sizes alike.
src/selection_tools.jl
Outdated
| @@ -252,7 +255,10 @@ end | |||
|
|
|||
|
|
|||
| """ | |||
| select_region!(src, dst=nothing; new_size=size(src), center=size(src).÷2 .+1, dst_center=nothing, pad_value=zero(eltype(mat), operator!=assign_to!)) | |||
| select_region!(srcsrc::AbstractArray{T, N}, dst=nothing; | |||
There was a problem hiding this comment.
That was a typo in the documentation. Fixed now. Also added test cases.
|
Please make a test for missing case: https://app.codecov.io/gh/bionanoimaging/NDTools.jl/pull/13/blob/src/selection_tools.jl |
|
Did you run the tests locally? I rerun them here. |
|
So ready to merge? |
|
Yes. I can merge, if you agree. |
fixed a bug in select_region, select_region! and added the dst_center argument to select_region_view.