Add is_binned property #3775
Add is_binned property #3775
Conversation
src/scipp/coords/transform_coords.py
Outdated
| if da.bins is not None: | ||
| da.bins.coords.pop(name, None) | ||
| if da.is_binned: | ||
| da.bins.coords.pop(name, None) # type: ignore[union-attr] |
There was a problem hiding this comment.
How about changing the type hint of bins right now, even if it may still return None. Then you can avoid suppressing everything (and having to remove it again later)?
There was a problem hiding this comment.
Could do. No downstream package actually passes type checks at the moment anyway.
src/scipp/core/bins.py
Outdated
| """ | ||
| Returns helper :py:class:`scipp.Bins` allowing bin-wise operations | ||
| to be performed or `None` if not binned data. | ||
| """ |
There was a problem hiding this comment.
Do you want to add a deprecation note for the None return behavior in the docstring?
There was a problem hiding this comment.
I was thinking of releasing this change, then adding a deprecation warning in the release after. Should I still add something in the docstring now?
There was a problem hiding this comment.
I don't think you can add a deprecation warning anyway, right? So why not put it into the docstring now?
There was a problem hiding this comment.
Why not? We can raise a warning when .bins is called on an unbinned object.
There was a problem hiding this comment.
But won't that give tons of false positives from uses such as
if da.bins is None:
# do a
else:
# do b?
There was a problem hiding this comment.
Or maybe the order of releases, refactoring, and deprecation we had in mind was mismatching, so it may just be down to confusion!
There was a problem hiding this comment.
My idea was just to insert an extra step by raising a warning and later change that to an error. This would allow people to still use the old approach before it gets properly disabled. But it would trip up our tests and force us to migrate.
src/scipp/core/bins.py
Outdated
| def _bins(obj: _O) -> Bins[_O]: | ||
| """Returns helper :class:`scipp.Bins` for bin-wise operations. | ||
| .. attention:: |
There was a problem hiding this comment.
Didn't Sphinx have a specific tag for deprecation?
First step towards #3687
Unfortunately, this leads to a bunch of type errors because Mypy does not understand the relationship between
.is_binnedand.bins. But once we makebinsraise, we can just revert the 'Suppress temporary type error' commit.