TEST: Check non-integral slopes, intercepts in ArrayProxy API #847
Conversation
Codecov Report
@@ Coverage Diff @@
## master #847 +/- ##
==========================================
+ Coverage 90.09% 90.09% +<.01%
==========================================
Files 98 98
Lines 12450 12452 +2
Branches 2188 2190 +2
==========================================
+ Hits 11217 11219 +2
Misses 883 883
Partials 350 350
Continue to review full report at Codecov.
|
| if np.can_cast(scl_slope, use_dtype): | ||
| scl_slope = scl_slope.astype(use_dtype) | ||
| if np.can_cast(scl_inter, use_dtype): | ||
| scl_inter = scl_inter.astype(use_dtype) |
There was a problem hiding this comment.
Unconditionally casting to int types would truncate the new parameters.
There was a problem hiding this comment.
So if it can't cast, but the user asks to, should this maybe spit a warning out?
There was a problem hiding this comment.
It's a helper method intended to do a best-effort downcast, but not to the detriment of accuracy. If you ask me to return ints but it would change the values (beyond a floating point rounding error), then the expected behavior is to keep floats. Spitting out a warning here would be extremely noisy.
| @@ -262,7 +266,7 @@ def sio_func(): | |||
| dtype=dtype, | |||
| dtype_out=dtype_out, | |||
| arr=arr.copy(), | |||
| arr_out=arr * slope + inter, | |||
| arr_out=arr.astype(dtype_out) * slope + inter, | |||
There was a problem hiding this comment.
If arr is float32, arr * slope + inter is float32. This hasn't mattered with the simple factors we've been using, but the new parameters produce different rounding errors in float32 (basic numpy coercion) and float64 (nibabel array scaling).
This change ensures that we produce our expected array in the wider dtype.
effigies
force-pushed
the
fix/slope_inter_guarantees
branch
from
014d676 to
79a4063
Compare
effigies
force-pushed
the
fix/slope_inter_guarantees
branch
from
79a4063 to
170365e
Compare
effigies
force-pushed
the
fix/slope_inter_guarantees
branch
from
170365e to
779f27b
Compare
effigies
changed the title
| if np.can_cast(scl_slope, use_dtype): | ||
| scl_slope = scl_slope.astype(use_dtype) | ||
| if np.can_cast(scl_inter, use_dtype): | ||
| scl_inter = scl_inter.astype(use_dtype) |
There was a problem hiding this comment.
So if it can't cast, but the user asks to, should this maybe spit a warning out?
|
Pretty happy with these tests (though they do add time... I probably don't need to do all the ints, uints, floats and complexfloats). These will help with stress-testing #844, so if there are no objections I'll merge tomorrow. |
Realized the ArrayProxy API tests are only using integral slopes and intercepts. Added some 5 s.f. parameters.
Currently breaks tests. Will look in detail when I can, but more eyes are always welcome.
This should be resolved and merged into #844, to ensure that the changes to results stay within tolerance.