Skip to content

ENH: add mm->q floordiv #12308

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 1, 2019
Merged

Conversation

tylerjereddy
Copy link
Contributor

@tylerjereddy tylerjereddy commented Nov 2, 2018

Motivated by #12120 (comment)

Add support for floor division between timedelta64 operands with type signature: mm->q

I tried mm->q for closer mimic with built-in timedelta, but that makes it tricky to handle NaT-> NaN.

Maybe there's a cleaner / more efficient way to deal with negative values.

@eric-wieser
Copy link
Member

Hmm, seems weird to be to have floordiv return float - I'm not sure if that's better or worse than losing NaTs.

@tylerjereddy
Copy link
Contributor Author

@shoyer Should I switch to returning an int and do something else with NaT?

@tylerjereddy
Copy link
Contributor Author

Based on in-person discussion with @shoyer & @eric-wieser I should likely revise this to return integer values.

For the case of NaT in the numerator, denominator, or both, use error flag to emit warning & return 0.

@shoyer
Copy link
Member

shoyer commented Dec 1, 2018 via email

@tylerjereddy tylerjereddy changed the title ENH: add mm->d floordiv ENH: add mm->q floordiv Dec 6, 2018
@tylerjereddy
Copy link
Contributor Author

Refactored to return integer type -- tests pass locally anyway. I wonder what reviewers will think about the way I intercepted floor division for type resolution between m8 types -- is the strcmp too fragile? It is readable at least.

@eric-wieser
Copy link
Member

Can you construct a test that would fail if we lost precision through a float?

@tylerjereddy tylerjereddy force-pushed the timedelta64_floordiv branch 2 times, most recently from 21ab174 to 3ed3494 Compare December 6, 2018 20:32
@tylerjereddy
Copy link
Contributor Author

Can you construct a test that would fail if we lost precision through a float?

Yes, I've added precision unit tests for both code paths based on integers that a double can't represent exactly.

Since NPY_MIN_LONGLONG is cast to NaT, its special case handling from the longlong inner loop was removed as well.

* add support for floor division between
timedelta64 (m8) operands with generic
or specific units; type signature is
mm->q
@mattip mattip merged commit 87e3409 into numpy:master Jan 1, 2019
@mattip
Copy link
Member

mattip commented Jan 1, 2019

Thanks Tyler

@tylerjereddy tylerjereddy deleted the timedelta64_floordiv branch January 2, 2019 17:42
@charris charris added the 09 - Backport-Candidate PRs tagged should be backported label Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants