Add test for core.nanops and fix several bugs in nanops
State: closed

This pull request accomplishes two main things:
1. It adds unit tests for `nanops`
2. It fixes several bugs identified by the unit tests

related #7352 
related #7353 
related #7354 
related #7357 


Labels:
name: Bug
name: Testing
description: pandas testing functions or related to the test suite
name: Dtype Conversions
description: Unexpected or buggy dtype conversions
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
how's this coming along?

I ran into one issue on python 3 that I can't figure out so I am expanding
the tests a little to try to figure out what is going on.

To be specific, currently datetime64 and timedelta dtypes are not tested.
Unfortunately, this seems to be where the new problem arises.  So I am
going to test those dtypes explicitly.

again, I would like you to break these into separate pieces, 1 issue per PR (sometimes its ok to have 2 or more issues in a single PR, but these are for VERY related things).

So what should I do about the unittests?  Either I have to submit a failing unit test, submit without any unit test until the end, or substantially alter the unit tests with each pull request.  I really don't want to do the last option, that is a lot of additional work for very little benefit, especially considering the git history will preserve all the individual commits anyway.  

That is the reason I submitted it all at once. The individual bugs are separate, but the unittests that expose them are more tightly intertwined.

why are the tests intertwined? you should have separate functions for each individual issue.

in your first PR, you setup the new structure of the tests. Then in each PR you add the tests that solve that issue. Each PR should have a set of tests in the first commit that fail. Then the fixes in the next commit. Build PR's on top of each other. Or submit a PR which changes the structure of the tests, but doesn't actually _do_ much, then each PR can build on that independently.

It will be MUCH easier for you (and us) if you do it this way; that way a PR fixes a single issue, can be reviewed realatively quickly and merged quickly. We DO NOT build big PR's here, except for a really big issue. This is simply 4 bug fixes. But its REALLY hard to tell as its SO intertwined.

The tests go through a lot of different combinations of parameters and test each one.  That is how I discovered the bugs to begin with, they caused a test that should be passing to fail.  Testing each individual combination separately is infeasible.  Only testing specific issues will miss regressions and will still require overly complicated tests related to what there is now.

ok. my point is that you are touching SO much code it is very hard to figure out what you are doing, and importantly are there side effects. Pls, pls try to limit what you are touching at a single go. I suspect you can disentangle these tests. Yes you are testing various combinations, and that is good. One way to do this then, is to put the test harness together as much as possible so it passes currently master. 

Then in each PR add the changes (dtypes, whatever), to make that issues fail, then fix them.

ok, so closing this a you are submitting individual PR's for the issues (tests already merged)