Repository navigation
Provide a properly public find_bin_edge_dims - #2469
Conversation
|
Is this referencing the correct issue? |
|
|
||
| configuration | ||
| logging | ||
| utils |
There was a problem hiding this comment.
Careful here: We have always considering functions in utils implementation details that can change at any time. If we now make all the functionality there "public" (documented) we would lose that ability to do so.
Have you considered other places for the added functionality? Or defining manually which functions are documented (and thus supported) publicly?
There was a problem hiding this comment.
Should we rename utils to _utils to indicate that it is non public?
Or defining manually which functions are documented (and thus supported) publicly?
I tried that because I didn't like that each util module is documented separately. But autosummary is pretty inflexible and doing this is kind of awkward.
Have you considered other places for the added functionality?
We could add bin_edges at the top level. We were considering a bin-edge toolkit at some point anyway. Although, we put that on ice for now, right?
There was a problem hiding this comment.
I think midpoints is top-level?
If we follow my thoughts below (making this a method of the coords class) then the problem vanishes anyway?
| """ | ||
| Miscellaneous utilities. | ||
|
|
||
| Note that all functions from submodules are also available in ``scipp.utils`` directly. | ||
| """ |
| return bin_edges | ||
|
|
||
|
|
||
| def is_bin_edges(ref: VariableLike, /, *, coord: Variable) -> bool: |
There was a problem hiding this comment.
- Missing a dimension label? What if
coordis 2d? The return value will be unclear. - Would this work better as a method of
Coord, i.e.,da.coords.is_edges(name, dim)?
There was a problem hiding this comment.
Missing a dimension label? What if coord is 2d? The return value will be unclear.
It checks if any dim is a bin-edge. I.e. the same that the HTML output does. I don't see why we would do anything different here.
Would this work better as a method of Coord, i.e., da.coords.is_edges(name, dim)?
Then it should also be a method of attrs and meta. And it would limit it to cases where the coord is already stored in the DataArray. What if a user wants to check before adding it because they maybe don't want to add bin-edges?
There was a problem hiding this comment.
It checks if any dim is a bin-edge. I.e. the same that the HTML output does. I don't see why we would do anything different here.
Because this is really important for writing code that is not broken? If we have 2d data and coord we need to know which dim has edges. For the HTML output it is maybe not critical, or rather it should be fixed to indicate which dims the edges refer to?
Then it should also be a method of attrs and meta.
Yes.
And it would limit it to cases where the coord is already stored in the DataArray. What if a user wants to check before adding it because they maybe don't want to add bin-edges?
Seems like a very special case? Maybe we should not compromise the API for this at this point?
There was a problem hiding this comment.
Finding out which dim has edges is covered by find_bin_edge_dims. If we add a dim arg to is_bin_edges, it should be optional for 1d coords, I think. Otherwise, one would pretty much always write is_bin_edges(da, da.coords['x'], da.coords['x'].dim) (or the equivalent method).
Maybe we should not compromise the API for this at this point?
I don't see this as a compromise. But I don't know of any concrete use case of a free function. So I am fine with changing it to a method.
There was a problem hiding this comment.
If we add a dim arg to is_bin_edges, it should be optional for 1d coords, I think.
Yes that makes sense. I believe we have done so in similar cases.
|
|
We should probably document them, yes. Can they not be listed in https://scipp.github.io/reference/classes.html? |
| for idx, dim in enumerate(var.dims): | ||
| length = var.shape[idx] | ||
| if not ds.dims: | ||
| # Have a scalar slice. | ||
| # Cannot match dims, just assume length 2 attributes are bin-edge | ||
| if length == 2: | ||
| bin_edges.append(dim) | ||
| elif dim in ds.dims and ds.shape[ds.dims.index(dim)] + 1 == length: | ||
| bin_edges.append(dim) | ||
| return bin_edges |
There was a problem hiding this comment.
Can this now use the newly bound C++ method?
There was a problem hiding this comment.
This would not be any less complex. It would have to check both meta and masks, except that Dataset does not have masks. So there needs to be quite a lot of logic.
| same_data = all(isclose(x.data, y.data, rtol=rtol, atol=atol, | ||
| equal_nan=equal_nan)).value if include_data else True |
There was a problem hiding this comment.
Unrelated, but am I missing something here or is this just allclose?
2 participants
Fixes #2468
There are a bunch of fixes to docstrings because those functions are now documented as part of the utils module and sphinx started complaining.