ENH: Implement divmod and remainder for timedelta and int #16458
Conversation
There was a problem hiding this comment.
I found running the pandas test suite against the NumPy feature branch helpful for these. Usually the impact was pretty easy to fix though & they seemed pretty keen to get the enhancements last time I believe.
The CI fail is unrelated--I wish we could get Azure to have a 32-bit mingw on the base VMs, that DL is an unfortunate bottleneck for NumPy/SciPy.
| return -1; | ||
| } | ||
| out_dtypes[1] = PyArray_DescrFromType(NPY_LONGLONG); | ||
| if (out_dtypes[1] == NULL) { |
There was a problem hiding this comment.
codecov seems to be showing useful patch diffs again at least--not sure if this conditional is too pathological to test, but shows up as uncovered
There was a problem hiding this comment.
Oh, is that for platforms that don't support that integer size or something? I guess NumPy probably only uses one or maybe two platforms for the cov reports in any case.
There was a problem hiding this comment.
Hmmm, I copy pasted this without thinking maybe, or just wrote it without thinking. This condition can never be hit, it would be an allocation error, but that is not even possible...
There was a problem hiding this comment.
@tylerjereddy does codecov only show them if it thinks there is a serious change? It seemed a bit random whether or not the patch diff shows up in the CI grid.
EDIT: Example for a missing one #16418
There was a problem hiding this comment.
Maybe we need another setting or two for it to always show the patch diff--let's ping @thomasrockhu from their team.
I believe if there are some failures in CI, this can affect the reporting status for codecov. When we turned it back on recently though, it did show a patch entry, and just said something like "no tracked files affected" because we only changed the codecov config file itself. Anyway, now I'm side-tracking your PR..
There was a problem hiding this comment.
Hi everyone, taking a look at this now. Just to make sure I'm getting this right, sometimes the patch status check isn't showing up on PRs? If there are failures in CI, Codecov normally doesn't report back, but that setting can be overwritten. This would prevent project from showing up as well, though.
There was a problem hiding this comment.
Yeah, I think project shows up now always (or almost always?), patch seemed to be missing sometimes, not sure if due to failures. Do failures in the azure pipeline count, since we have some there fairly regularly unfortunately.
There was a problem hiding this comment.
So looks like for #16418, patch had still been set to false, and that's why no patch status showed up.
However, for more recent PRs that have a successful CI, I'm seeing a patch status. We, unfortunately, can't distinguish between what is and what isn't a CI status, so failures in Azure Pipelines do count. If you would prefer not to have this behavior, you can edit the yaml with
codecov:
require_ci_to_pass: false
| @@ -1264,6 +1264,13 @@ PyUFunc_RemainderTypeResolver(PyUFuncObject *ufunc, | ||
| out_dtypes[2] = out_dtypes[0]; | ||
| Py_INCREF(out_dtypes[2]); | ||
| } | ||
| else if (PyTypeNum_ISINTEGER(type_num2)) { | ||
| out_dtypes[0] = PyArray_DESCR(operands[0]); |
There was a problem hiding this comment.
Note to self, this needs an ensure_dtype_nbo.
Also fixes that we should not be giving warnings when the input is already a NaT.
|
Nice, thanks for pushing forward on this |
Also fixes that we should not be giving warnings when the input is already a NaT.
@jbrockmendel maybe you can have a look as well, whether this seems right? Maybe @tylerjereddy if you have the time, since the initial implementation was from you.
Marking as draft, although its probably pretty far. This probably needs one more iteration, possibly a few more tests.
I may have went down a wrong path when modifying/trying to expand the tests and get the warnings right, it did get somewhat messy. It combines all into one giant test now (including the integers), since we can do at least partial cross checks.
One additional test that may be good, is a test for the integer types other than int64, and I have not yet checked carefully if stealing some pandas tests may be nice.
TODO: