fix: add appropriate type combinations #3250
Conversation
SimonHeybrock
left a comment
There was a problem hiding this comment.
Is there a reason why float was excluded in the original arg_list?
No, just an oversight. Unfortunately Scipp's dtype handling mechanism requires being explicit about what is allowed. In many cases this is good, but we have also repeatedly run into "bugs" like these, where something that should work did not.
| arg_list<bin_range_arg<double, double>, bin_range_arg<double, float>, | ||
| bin_range_arg<double, int32_t>, bin_range_arg<double, int64_t>, | ||
| bin_range_arg<float, double>, bin_range_arg<float, float>, | ||
| bin_range_arg<float, int32_t>, bin_range_arg<float, int64_t>, | ||
| bin_range_arg<int32_t, double>, bin_range_arg<int32_t, float>, | ||
| bin_range_arg<int32_t, int32_t>, bin_range_arg<int32_t, int64_t>, | ||
| bin_range_arg<int64_t, double>, bin_range_arg<int64_t, float>, | ||
| bin_range_arg<int64_t, int32_t>, bin_range_arg<int64_t, int64_t>, |
There was a problem hiding this comment.
I think this is probably fine, but have you considered and checked if things can go wrong (such as comparisons) for certain type combinations, which might lead to, e.g., unintentionally dropping parts of the data?
| arg_list<bin_range_arg<double, double>, bin_range_arg<int64_t, double>, | ||
| bin_range_arg<int64_t, int64_t>, bin_range_arg<int32_t, int32_t>, | ||
| bin_range_arg<int32_t, double>, | ||
| arg_list<bin_range_arg<double, double>, bin_range_arg<double, float>, |
There was a problem hiding this comment.
Can you also add a (or a couple of) unit test(s)?
There was a problem hiding this comment.
Added a TYPED_TEST_SUITE, it doesn't account for all combinations.
There was a problem hiding this comment.
I don't think this is testing much: The bin algorithm is quite complex, calling a whole series of functions like this one. So in this case this test is mostly just "noise", since it provides no guarantee that the higher-level functionality works.
At a minimum, we should add a (Python) test that tests the full case reported in the linked issue.
src/scipp/data/__init__.py
Outdated
| x = array( | ||
| dims=['row'], unit='m', values=rng.random(nrow) * coord_max, dtype=coord_dtype | ||
| ) | ||
| y = array( | ||
| dims=['row'], unit='m', values=rng.random(nrow) * coord_max, dtype=coord_dtype | ||
| ) | ||
| z = array( | ||
| dims=['row'], unit='m', values=rng.random(nrow) * coord_max, dtype=coord_dtype | ||
| ) |
There was a problem hiding this comment.
Not sure it matters much in practice, but now this function will be slower, even with the default coord_max. Unless you think this is more generally useful, maybe you can keep the scaling in the unit test where it is required?
There was a problem hiding this comment.
I think it might be useful when testing with integer coordinates.
But I'll change it so it doesn't increase the runtime when used without scaling.
6be69e7 to
da4e811
Compare
da4e811 to
48409cb
Compare
Fixes #3176.
Is there a reason why
floatwas excluded in the originalarg_list?Now I just added all type combinations between
double, float, int32_t, int64_t.