Support data arrays in where #3675
Conversation
tests/core/operations_test.py
Outdated
| ) | ||
|
|
||
| # condition is missing coord | ||
| with pytest.raises(sc.DatasetError): |
There was a problem hiding this comment.
Suggest using
| with pytest.raises(sc.DatasetError): | |
| with pytest.raises(sc.DatasetError, match='<something>'): |
especially since we are trying to detect multiple different exceptions.
lib/python/operations.cpp
Outdated
| apply_where(c_data, x_data, y_data, c_coords, x_coords, y_coords); | ||
|
|
||
| if (x_coords.has_value()) { | ||
| return py::cast(DataArray(new_data, x_coords.value(), {})); |
There was a problem hiding this comment.
Maybe I missed something, but it is not clear to me why we require c_coords to be a superset, but then drop those coords and use only value coords?
There was a problem hiding this comment.
I'm not sure about this either. I thought that we want to make sure that the condition's coords match the data. But it seemed too restrictive to require completely identical coords. Did I get it the wrong way around? Should the data's coords be a superset of the condition's?
There was a problem hiding this comment.
Could we just return the union? Or require that the condition is a variable?
There was a problem hiding this comment.
Could we just return the union?
I guess we could. It's not exactly cheap to do a union of 3 inputs but it can be done.
Or require that the condition is a variable?
I wouldn't do that because it encourages discarding coords from the inputs when building the condition.
There was a problem hiding this comment.
Could we just return the union?
I guess we could. It's not exactly cheap to do a union of 3 inputs but it can be done.
I thought we should require that value coords are the same (like you do already), but return the union of condition and value coords? Similar to how regular binary ops do it (there should be helpers for that somewhere).
| # differing values | ||
| with pytest.raises(sc.DatasetError): | ||
| sc.where(condition, x, y) | ||
|
|
||
| # differing values | ||
| with pytest.raises(sc.DatasetError): | ||
| sc.where(condition, x, y) |
There was a problem hiding this comment.
Duplicate? And missing match.
Fixes #3648
I disallowed masks in all operands because supporting them is complicated. I think the output
result.masks.keys()should be the union of all input masks. But the masks ofxandyshould not be or-ed. Instead, their elements should be selected based on the condition and then or-ed with the condition's masks.