MAINT: Remove unhelpful docstrings, improve helpful ones #9459
Conversation
eric-wieser
added
04 - Documentation
03 - Maintenance
component: numpy.ma
| if domain is not None: | ||
| m |= domain(da, db) | ||
| if self.domain is not None: | ||
| m |= self.domain(da, db) |
There was a problem hiding this comment.
The one actual code change. We don't need to look up domain with ufunc_domain.get(self.f, None), because we set that in the constructor to self.domain
|
|
||
| ``_DomainCheckInterval(a,b)(x)`` is False where ``a <= x <= b``. |
There was a problem hiding this comment.
Not sure this is really the same as the old comment because of nans. The old comment more literally describes which ufuncs are used in __call__ (less, logical_or, greater).
That is, x < a is not the opposite of a <= x for x = np.nan.
There was a problem hiding this comment.
Also, while we're updating the comment, I'm not sure I like "define a valid interval". maybe "define an interval of validity"?
There was a problem hiding this comment.
Hmm, you're right about nans - this should read "where ... or isnan(x)" to be correct, right?
I think that's actually a bug in the existing code - that means all functions consider nan in their domain, which seems wrong to me. I don't know whether this is actually externally visible
There was a problem hiding this comment.
Actually, if we think of using DomainCheckInterval even more generally - a, x and b don't even need to be numbers, so adding or isnan(x) might not be enough either. I think they can be any objects that can go through numpy ufuncs.
I think in practice none of this matters since the only "user" of DomainCheckInterval is the MaskedArray code which presumably masks Nans, and only calls it for numeric types. Nevertheless I don't think it's a good idea to make the comment less technically accurate than it was before.
There was a problem hiding this comment.
I think the way forward might be to update the implementation to match these new comments - the only reason it's done the way it is is to avoid doing an not operation, which seems to come at the cost of nan-correctness
There was a problem hiding this comment.
That seems reasonable to me, assuming it doesn't break any tests.
|
What should we do with this? |
|
I don't think it's ready at the moment |
|
@eric-wieser Still not ready? Looks like a minor improvement, larger improvements start to get deeper into the ma code ... |
|
I'd forgotten about this one. I'll try to come back to it in the next few days |
|
@eric-wieser would you like help moving this forward? It seems what is missing is
|
|
@eric-wieser ping |
|
I'm going to close this, needs rebase in any case. @eric-wieser Please open a new PR if you want to pursue this. |
"Execute the call behavior." is not a helpful description for what
__call__does