Add test for core.nanops and fix several bugs in nanops #7358
Conversation
| @@ -286,6 +293,9 @@ def get_median(x): | |||
| if values.dtype != np.float64: | |||
| values = values.astype('f8') | |||
|
|
|||
| if axis is None: | |||
| values = values.flatten() | |||
There was a problem hiding this comment.
use .ravel() doesn't copy unless necessary (flatten always does)
There was a problem hiding this comment.
I think axis=None is an assertion error
There was a problem hiding this comment.
Sorry, I changed that in a previous version but the change got lost when I split the pull requests. I will fix that now.
There was a problem hiding this comment.
As I showed in the corresponding issue #7352, this works fine in bottleneck, so it should probably work here too.
There was a problem hiding this comment.
Also, all of the other cases I tested worked fine when axis=None, this was the only case where that didn't work as expected.
|
how's this coming along? |
|
I ran into one issue on python 3 that I can't figure out so I am expanding To be specific, currently datetime64 and timedelta dtypes are not tested. |
|
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. |
… arrays ( issue #7357 )
|
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) |
This pull request accomplishes two main things:
nanopsrelated #7352
related #7353
related #7354
related #7357