Repository navigation
Lookup: fill Eigen types with NaN - #3892
Conversation
| Variable fill = fill_value.value_or(zero_like(function.data())); | ||
| Variable fill = | ||
| fill_value | ||
| .or_else([&] { return std::optional{zero_like(function.data())}; }) |
There was a problem hiding this comment.
Can you explain what this change does? I did not follow.
There was a problem hiding this comment.
It's an optimisation. The old code always constructed the default value zero_like(function.data())). The new code only does that when no value is provided. (Using C++23's or_else)
| sc.testing.assert_identical(sc.lookup(da)(var), expected) | ||
|
|
||
|
|
||
| def test_histogram_default_fill_value_vector() -> None: |
There was a problem hiding this comment.
Also test Matrix3d and Affine3d?
Is it easy to just parametrize?
There was a problem hiding this comment.
I parametrized the test. But we don't even support transformations in lookup. That is not part of this PR, so should I remove the fill value support for Matrix3d and Affine3d again?
There was a problem hiding this comment.
should I remove the fill value support for Matrix3d and Affine3d again?
I don't mind either way
| assert sc.identical(sc.lookup(da, mode='previous', fill_value=fill)(var), expected) | ||
|
|
||
|
|
||
| def test_previous_vector() -> None: |
There was a problem hiding this comment.
This test seems unrelated to the 0 vs nan behaviour, but increases test coverage, right?
5ff220b to
7457cdf
Compare
|
|
||
| var = sc.array(dims=['event'], values=[-1, 2]) | ||
| expected = sc.empty(dims=['event'], shape=[2], dtype=dtype) | ||
| expected.values[...] = fill # sc.full does not work for vector3 |
There was a problem hiding this comment.
sc.full does not work for vector3
Should we open an issue?
7457cdf to
846a042
Compare
2 participants
Fixes #3860