[BUG] Preserve float32 dtype in affected transformers. - #3752
Conversation
baraline
left a comment
There was a problem hiding this comment.
Thanks for working on this, the issue is a bit more complex than simply casting the output, we want the internal computation to be performed in the appropriate precision, to benefit from the reduced memory footprint.
I didn't comment on _dwt, _slope and _pla as the issues are the same that those pointed out in intervals.
If you need any pointers don't hesitate to ask questions.
| [ | ||
| (np.float32, np.float32), | ||
| (np.float64, np.float64), | ||
| (np.int64, np.float64), |
There was a problem hiding this comment.
need to also add int32, float64
| return Xt | ||
| out_dtype = np.float32 if X.dtype == np.float32 else np.float64 | ||
| return Xt.astype(dtype=out_dtype, copy=False) | ||
|
|
There was a problem hiding this comment.
We shouldn't neet to cast the input here, it must natively used float32 (or 64) in the generate intervals function.
What we want is that if we have a float 32 input, computation are made on float 32 to benefit from the reduced memory footprint (and computation time to some extent)
There was a problem hiding this comment.
I removed final cast. The input is promoted once and interval buffers now use X.dtype, so float32 is preserved throughout the SupervisedIntervals.
| out_dtype = np.float32 if X.dtype == np.float32 else np.float64 | ||
| Xt = np.zeros((X.shape[0], len(transform)), dtype=out_dtype) |
There was a problem hiding this comment.
Same comment as above, we want the interval computation to use 32 bit precision, not simply cast the output.
So _transform_intervals itself should return the appropriate dtype already.
There was a problem hiding this comment.
_transform_intervals now returns values in the working X.dtype, and _transform allocates its result directly with that dtype instead of correcting the dtype
| [ | ||
| (np.float32, np.float32), | ||
| (np.float64, np.float64), | ||
| (np.int64, np.float64), |
There was a problem hiding this comment.
similar to before, add int32, float64
| ) | ||
| def test_dwt_preserves_float_precision(dtype, expected_dtype): | ||
| """Check dwt preserves float32 and promotes proper dtype output.""" | ||
| X = np.arange(16, dtype=dtype).reshape(2, 1, 8) |
There was a problem hiding this comment.
use the testing utils functions to generate testing data, as you did in the inerval tests (make_example_3d_numpy)
| [ | ||
| (np.float32, np.float32), | ||
| (np.float64, np.float64), | ||
| (np.int64, np.float64), |
| [ | ||
| (np.float32, np.float32), | ||
| (np.float64, np.float64), | ||
| (np.int64, np.float64), |
| X = np.array( | ||
| [[[4, 6, 10, 12, 8, 6, 5, 5]]], | ||
| dtype=dtype, | ||
| ) |
There was a problem hiding this comment.
again, use the testing utils to generate data
There was a problem hiding this comment.
Updated to use make_example_3d_numpy
| np.array([[0, 1, 2, 3]], dtype=dtype), | ||
| np.array([[0, 2, 4, 6, 8]], dtype=dtype), |
There was a problem hiding this comment.
I don't think the current testing util to generate variable length list allows dtype args, so this one is fine for now.
|
Thank you for the review! I updated implementation so that computations itself preserve float32 within in |
|
Great ! Overall looks fine but I'll try to have a closer look at it this week. Thanks for continuing to work on this. |
|
@MatthewMiddlehurst, do you think it would be worth having a internal estimator attribute (would tag system be appropriate to declare the capability ?) with the input precision (32/64) set during fit and reused across all the estimator declarations ? We can set it during input preprocessing similarly to how @4nmus wrote it in the test : |
Reference Issues/PRs
Fixes part of #3723.
What does this implement/fix? Explain your changes.
Preserves
float32output precision in the following transformers:DWTTransformer
Resizer
SlopeTransformer
SupervisedIntervals
PLASeriesTransformer
Previously, these transformers could unnecessarily promote float32 input to float64 through explicit casts or default NumPy allocations.
The changes preserve the following dtype behavior:
-float32 input produces float32 output
-float64 input produces float64 output
-integer input is promoted to float64
Regression tests were added or updated to verify dtype behavior for float32, float64, and integer inputs.
ClaSPTransformer, which is mentioned in #3723, is not included in this PR because its existing float64 conversion is related to a separate Numba typing constraint.
Does your contribution introduce a new dependency? If yes, which one?
No.
Any other comments?
ClaSPTransformer still returns float64 since it forces the input to be float64 due to Numba.
PR checklist
For all contributions
For new estimators and functions
__maintainer__at the top of relevant files and want to be contacted regarding its maintenance. Unmaintained files may be removed. This is for the full file, and you should not add yourself if you are just making minor changes or do not want to help maintain its contents.For developers with write access