NaT comparison change breaks pandas test suite

[shoyer]

xref pandas-dev/pandas#12049

The source issue is changing NaT comparison (#7001)

I'll take a look and see if this is user facing or just in the test suite. If it does effect users, we may need to do a deprecation cycle instead.

[charris]

Thanks for testing that. Better now than later...

[shoyer]

Unfortunately, this change breaks several user facing APIs in pandas. We'll need to roll this change back for the NumPy v1.11 release, and preferably insert a deprecation warning in its place.

[jreback]

pandas side fixes are actually pretty easy pandas-dev/pandas#12058 (and compat across numpy versions)

but I agree with @shoyer you might consider providing a FutureWarning for a version so people can take action :)

[njsmith]

Yeah, that'd probably be more virtuous of us anyway...

On Fri, Jan 15, 2016 at 1:15 PM, Jeff Reback notifications@github.com
wrote:

pandas side fixes are actually pretty easy pandas-dev/pandas#12058
pandas-dev/pandas#12058 (and compat across numpy
versions)

but I agree with @shoyer https://github.com/shoyer you might consider
providing a FutureWarning for a version so people can take action :)

—
Reply to this email directly or view it on GitHub
#7019 (comment).

Nathaniel J. Smith -- http://vorpus.org

[charris]

@jreback We are going to an accelerated release schedule. How long a time would you like to see a FutureWarning?

[jreback]

@charris

ideally only 1 major release cycle. what is your expected cycle time for major releases?

we have not been good about that and have been leaving FutureWarnings open way to long IMHO. (going to fix this soon), e.g. pandas-dev/pandas#6581

[charris]

@shoyer Do you want to take care of this? Sounds like we need to revert the merge and put in place a FutureWarning.

[charris]

If not, let me know and I'll do it.

[shoyer]

Is there a cleaner way to handle this than putting the deprecate inside that ufunc kernel? That seems likely to have performance consequences.

On Fri, Jan 15, 2016 at 5:27 PM, Charles Harris notifications@github.com
wrote:

If not, let me know and I'll do it.

Reply to this email directly or view it on GitHub:
#7019 (comment)

[charris]

Oh, that's nasty. We don't want it in the loop if it can be helped, we would need to acquire the GIL. OTOH, we would need to check for NAT anyway if we want the warning to be specific. The only thing that occurs to me is to maybe make a wrapper function that shadows the true function, but that could be tricky. what with promotions and all and probably wouldn't make things any faster. Or we could just live with it for a release cycle. Hmm...

If anyone has ideas don't hesitate to make a suggestion.

[charris]

what is your expected cycle time for major releases?

We are shooting for 3-4 months.

[njsmith]

I guess we should just put it in the inner loop and pay the cost -- we'll
only pay an extra penalty when the ufunc execution requires multiple passes
through the inner loop, and that shouldn't be common, and we attempt to
minimize passes in any case to amortize their cost?

On Fri, Jan 15, 2016 at 6:34 PM, Charles Harris notifications@github.com
wrote:

what is your expected cycle time for major releases?

We are shooting for 3-4 months.

—
Reply to this email directly or view it on GitHub
#7019 (comment).

Nathaniel J. Smith -- http://vorpus.org

[charris]

This is what I'm thinking at the moment

• revert API: make all comparisons with NaT false #7001 before branching
• put future warning in loop before branching 1.11
• put API: make all comparisons with NaT false #7001 back in for 1.13, which should release Aug-Sept.

[njsmith]

Sounds good to me
On Jan 16, 2016 09:46, "Charles Harris" notifications@github.com wrote:

This is what I'm thinking at the moment

• revert API: make all comparisons with NaT false #7001 API: make all comparisons with NaT false #7001 before
  branching
• put future warning in loop before branching 1.11
• put API: make all comparisons with NaT false #7001 API: make all comparisons with NaT false #7001 back in for
  1.13, which should release Aug-Sept.

—
Reply to this email directly or view it on GitHub
#7019 (comment).

[shoyer]

@charris Yes, I think that's what needs to be done. If you're able to do that quickly that would be appreciated -- if not I should be able to find some time to fix this tomorrow.

[charris]

I can do the reversion tonight, but I'm not going to try anything that needs the little gray cells. If you can't get to it tomorrow let me know and I'll give it a shot. The 1.11 branch may be a day or two late in any case.

[charris]

@shoyer How does your time look? I can do the FutureWarning this afternoon if you don't have the time.

[charris]

Just for reference

#define NPY_ALLOW_C_API_DEF  PyGILState_STATE __save__;
#define NPY_ALLOW_C_API      do {__save__ = PyGILState_Ensure();} while (0);
#define NPY_DISABLE_C_API    do {PyGILState_Release(__save__);} while (0);

[charris]

See #7057.

[charris]

On track for 1.13, closing this.