BUG: rolling with MSVC 2017 build
State: closed

- [x] closes #21786
- [x] tests added / passed
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

For now just trying 3.7 on appveyor, not sure if all necessary conda packages are there

Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations
name: Windows
description: Windows OS
name: Window
description: rolling, ewma, expanding

Comments:
The `c3i_test` channel may have some missing packages.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=h1) Report
> Merging [#21813](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/0828c25913b5862d70322471a6ee4fa11d8f890d?src=pr&el=desc) will **increase** coverage by `<.01%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/21813/graphs/tree.svg?width=650&height=150&token=eZ4WkYLtcO&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #21813      +/-   ##
==========================================
+ Coverage   91.99%      92%   +<.01%     
==========================================
  Files         167      170       +3     
  Lines       50578    50553      -25     
==========================================
- Hits        46530    46510      -20     
+ Misses       4048     4043       -5
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.4% <ø> (ø)` | :arrow_up: |
| #single | `42.21% <ø> (+0.04%)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/io/formats/console.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvY29uc29sZS5weQ==) | `65.15% <0%> (-4.55%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `97.19% <0%> (ø)` | :arrow_up: |
| [pandas/core/generic.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ2VuZXJpYy5weQ==) | `96.47% <0%> (ø)` | :arrow_up: |
| [pandas/core/internals/blocks.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2Jsb2Nrcy5weQ==) | `94.46% <0%> (ø)` | |
| [pandas/core/internals/concat.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL2NvbmNhdC5weQ==) | `98.37% <0%> (ø)` | |
| [pandas/core/internals/managers.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW50ZXJuYWxzL21hbmFnZXJzLnB5) | `96.52% <0%> (ø)` | |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.37% <0%> (ø)` | :arrow_up: |
| [pandas/core/arrays/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYXJyYXlzL2NhdGVnb3JpY2FsLnB5) | `95.95% <0%> (ø)` | :arrow_up: |
| [pandas/io/formats/excel.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvZXhjZWwucHk=) | `97.39% <0%> (+0.01%)` | :arrow_up: |
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `94.16% <0%> (+0.05%)` | :arrow_up: |
| ... and [3 more](https://codecov.io/gh/pandas-dev/pandas/pull/21813/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=footer). Last update [0828c25...5d40eae](https://codecov.io/gh/pandas-dev/pandas/pull/21813?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Well, good news is we got a repro of this on our CI - it's not a python 3.7 issue, but instead a Visual Studio 2017 one.
https://ci.appveyor.com/project/pandas-dev/pandas/build/1.0.15961/job/r9axp415no6jubnr#L2059

Still not convinced this isn't a compiler bug, but having trouble to get a simple case to reproduce - one option would be to build our wheels VS 2015 - but annoying, b/c python forces the newest version, so can't have 2017 installed.  I'll keep trying some things.
This is kind of horrible, but it does fix the problem (just a lint issue I pushed for) and I'm out of ideas - any thoughts or suggestions @jreback, @TomAugspurger ?
@chris-b1 could you summarize the issue? You think MSVC is just optimizing away `if val == val`, which isn't true when `val` is NaN?
That's right, optimizing maybe not the right word, but somehow the `add_var` function is being turned into a no-op, the `mean_x`, `ssqdm_x`, and `nobs` variables don't get updated.  Adding any kind of unconditional action to the function fixes it - originally discovered by noticing it works with a print statement thrown in.

And yes, `val==val` is a not-a-NaN check.  We use it all over the place in this module, so it's not as if that check is universally being treated wrong, but something about this particular context makes MSVC think it's unreachable or something.
Ok, thanks...

@jjhelmus do we know anyone at Microsoft who works on their compilers that might be able to provide insight on https://github.com/pandas-dev/pandas/pull/21813#issuecomment-404501626 and https://github.com/pandas-dev/pandas/pull/21813#issuecomment-404571051?
@zooba might be able to connect you to the right team at Microsoft
I can try, but I haven't been having as much luck recently.

Since this is Cython, can someone post the generated code for the comparison? There's a much better chance of getting something useful if they're looking at the code they actually compile, and not the code someone else is compiling.
It's Cython, so of course the bigger file is a tangled mess, but the particular function is easy to read - lines of gist here.  
https://gist.github.com/chris-b1/cd974fb6b4165285a48b6d6c12129b40#file-window-cpp-L11028-L11096

Called in a few places, but for instance, here is a problem one.
https://gist.github.com/chris-b1/cd974fb6b4165285a48b6d6c12129b40#file-window-cpp-L11948

Cython isn't doing anything particular interesting - it's a `cdef` function so basically translates line for line into c++.

I can post it later, not on this machine, but I've also tried to write a simpler, plain c++ example that triggers the problem, but it always worked - really not sure why.
I can reproduce it with this snippet (has a few tricks to try and defeat optimisations, but they are likely unnecessary):

```c
#include <math.h>
#include <stdio.h>

double get_value(int i) {
    return i > 1 ? NAN : 7;
}

int check_default(double v1, double v2) {
    return ((v1 == v2) != 0);
}

#pragma float_control(precise, off, push)
int check_fast(double v1, double v2) {
    return ((v1 == v2) != 0);
}
#pragma float_control(pop)


int main(int argc, char **argv) {
    double x1 = get_value(argc);
    double x2 = get_value(argc);
    fprintf(stdout, "default: %d\nfast:    %d\n",
        check_default(x1, x2), check_fast(x1, x2)
    );
}
```

The comparison doesn't behave properly when you disable precise floating point operations. You may be doing this in the build with `/fp:fast` (if you build this snippet with that option, both checks return '1'; if you build with the default `/fp:precise`, it works). In fact, the [docs for this flag](https://docs.microsoft.com/en-us/cpp/build/reference/fp-specify-floating-point-behavior) specifically call out NaN comparisons as something that only work in `precise` mode.

So I'd guess you need to update the compiler flags for this one, or possibly add an explicit `isnan()` call.
Is the MSVC info something that can be inferred from platform.uname (at compile-time)?
Thanks for taking a look at this @zooba - unfortunately that's not exactly the same problem I'm seeing here, the compiled code is acting as if `(v1 == v2)` always evaluates to False/0.  We are building with `/fp:precise` (via defaults, but I've also tried manually overriding)
Interestingly using `isnan` from <math.h> does solve the problem here - still feels like MSVC is doing something wrong in the former case, but that's a reasonable enough workaround.
> the compiled code is acting as if `(v1 == v2)` always evaluates to False/0

Do you have a disassembly showing this? This would be very strange to see as a compiler optimization (unless you mean `(v == v)` always evaluates to True/1?)

> b/c python forces the newest version, so can't have 2017 installed

Just saw this while rereading the thread. If you start with all the tools on PATH and set `DISTUTILS_USE_SDK=1` then it'll skip the autodetection and use whatever you provide. (And if you help encourage the ecosystem to get off distutils completely, then more of these problems go away :) )
Could this SO answer be relevant?

https://stackoverflow.com/questions/8200311/checking-for-nan-in-cython/20031818#20031818
@zooba - do you know how to pull symbols into the dissambly from a `.pyd`?  I was playing around with dumpbin / compile flags, but never got anything better than, e.g. below, where everything is still in offsets.

```
 dumpbin.exe /DISASM pandas\_libs\window.cp36-win_amd64.pyd /OUT:tmp.txt
```

![image](https://user-images.githubusercontent.com/1924092/42768413-10da394c-88e5-11e8-85a7-6a416046762e.png)

> do you know how to pull symbols into the dissambly from a .pyd

Answering my own question - I was missing `'/DEBUG'` on `extra_link_args`
Alright @zooba - I  think I have this to a reasonable bit of a dis-assembly - pushing the bounds of my knowledge but looks like a code-generation bug.

Here's MSVC 2015 (printfs surround a call to `add_var`)  which works correctly.  In particular the highlighted lines are the NaN check.
https://gist.github.com/chris-b1/1049fdabafc56b2a25de7ad377016baa#file-msvc2015-asm-L6-L8

And here is MSVC 2017. The function call doesn't get inlined - half of the NaN check gets done (`jn`) - then the stack is being set up, then the other half the check (`jne`).  I believe in setting up the stack, the ZF flag originally set by `ucomisd` gets set back to 0 (from the `sub`) so the `jne` check will always be true, and the function unconditionally exits early.
https://gist.github.com/chris-b1/199f818c1fbb8f8c4108aa9e430df4f7#file-msvc2017-asm-L15-L19



@chris-b1 Yep, you're absolutely right. Thanks for tracking that down, I'll pass it on!
Here is the public side of the report: https://developercommunity.visualstudio.com/content/problem/294290/incorrect-codegen-for-double-equality.html Feel free to add pings there whenever you like
Thanks @zooba!

and thanks @jbrockmendel - that's what I'll have to do to fix the 2.7 build, more modern MSVCs are basically standards compliant and have `isnan` where it should be.
does using the fix proposed in #21940 help here? e.g. ``npy_isnan``?
> does using the fix proposed in #21940 help here? e.g. npy_isnan?

It does (latest commit has that and is passing on appveyor) - but less sure we want to apply that universally, I'll try perf on @jbrockmendel PR later, suspect it may be worse on Windows.

@jreback  - any idea on the circle CI failure? seems unrelated, but consistently failing on this branch

<details>

```bash
__________________ TestPythonParser.test_dtype_with_converter __________________

self = <pandas.tests.io.parser.test_parsers.TestPythonParser object at 0x7f2235d65b70>

    def test_dtype_with_converter(self):
        data = """a,b
    1.1,2.2
    1.2,2.3"""
        # dtype spec ignored if converted specified
        with tm.assert_produces_warning(ParserWarning):
            result = self.read_csv(StringIO(data), dtype={'a': 'i8'},
>                                  converters={'a': lambda x: str(x)})

pandas/tests/io/parser/dtypes.py:340: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../miniconda3/envs/pandas/lib/python3.5/contextlib.py:66: in __exit__
    next(self.gen)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

expected_warning = <class 'pandas.errors.ParserWarning'>
filter_level = 'always', clear = None, check_stacklevel = True

    @contextmanager
    def assert_produces_warning(expected_warning=Warning, filter_level="always",
                                clear=None, check_stacklevel=True):
        """
        Context manager for running code expected to either raise a specific
        warning, or not raise any warnings. Verifies that the code raises the
        expected warning, and that it does not raise any other unexpected
        warnings. It is basically a wrapper around ``warnings.catch_warnings``.
    
        Parameters
        ----------
        expected_warning : {Warning, False, None}, default Warning
            The type of Exception raised. ``exception.Warning`` is the base
            class for all warnings. To check that no warning is returned,
            specify ``False`` or ``None``.
        filter_level : str, default "always"
            Specifies whether warnings are ignored, displayed, or turned
            into errors.
            Valid values are:
    
            * "error" - turns matching warnings into exceptions
            * "ignore" - discard the warning
            * "always" - always emit a warning
            * "default" - print the warning the first time it is generated
              from each location
            * "module" - print the warning the first time it is generated
              from each module
            * "once" - print the warning the first time it is generated
    
        clear : str, default None
            If not ``None`` then remove any previously raised warnings from
            the ``__warningsregistry__`` to ensure that no warning messages are
            suppressed by this context manager. If ``None`` is specified,
            the ``__warningsregistry__`` keeps track of which warnings have been
             shown, and does not show them again.
        check_stacklevel : bool, default True
            If True, displays the line that called the function containing
            the warning to show were the function is called. Otherwise, the
            line that implements the function is displayed.
    
        Examples
        --------
        >>> import warnings
        >>> with assert_produces_warning():
        ...     warnings.warn(UserWarning())
        ...
        >>> with assert_produces_warning(False):
        ...     warnings.warn(RuntimeWarning())
        ...
        Traceback (most recent call last):
            ...
        AssertionError: Caused unexpected warning(s): ['RuntimeWarning'].
        >>> with assert_produces_warning(UserWarning):
        ...     warnings.warn(RuntimeWarning())
        Traceback (most recent call last):
            ...
        AssertionError: Did not see expected warning of class 'UserWarning'.
    
        ..warn:: This is *not* thread-safe.
        """
        with warnings.catch_warnings(record=True) as w:
    
            if clear is not None:
                # make sure that we are clearning these warnings
                # if they have happened before
                # to guarantee that we will catch them
                if not is_list_like(clear):
                    clear = [clear]
                for m in clear:
                    try:
                        m.__warningregistry__.clear()
                    except Exception:
                        pass
    
            saw_warning = False
            warnings.simplefilter(filter_level)
            yield w
            extra_warnings = []
    
            for actual_warning in w:
                if (expected_warning and issubclass(actual_warning.category,
                                                    expected_warning)):
                    saw_warning = True
    
                    if check_stacklevel and issubclass(actual_warning.category,
                                                       (FutureWarning,
                                                        DeprecationWarning)):
                        from inspect import getframeinfo, stack
                        caller = getframeinfo(stack()[2][0])
                        msg = ("Warning not set with correct stacklevel. "
                               "File where warning is raised: {actual} != "
                               "{caller}. Warning message: {message}"
                               ).format(actual=actual_warning.filename,
                                        caller=caller.filename,
                                        message=actual_warning.message)
                        assert actual_warning.filename == caller.filename, msg
                else:
                    extra_warnings.append((actual_warning.category.__name__,
                                           actual_warning.message,
                                           actual_warning.filename,
                                           actual_warning.lineno))
            if expected_warning:
                msg = "Did not see expected warning of class {name!r}.".format(
                    name=expected_warning.__name__)
                assert saw_warning, msg
            assert not extra_warnings, ("Caused unexpected warning(s): {extra!r}."
>                                       ).format(extra=extra_warnings)
E           AssertionError: Caused unexpected warning(s): [('ResourceWarning', ResourceWarning("unclosed file <_io.BufferedReader name='/tmp/tmpuehs2h5ztest_file.zip'>",), '/home/ubuntu/miniconda3/envs/pandas/lib/python3.5/importlib/_bootstrap.py', 967)].

pandas/util/testing.py:2535: AssertionError
```
</details>
I've had similar experiences recently (although it always happened to me on Travis) - I found the following issue and commented there #13962.
FYI @chris-b1 circle CI has a CLI for running stuff locally. I think it's safe to ignore here though.
@jreback - I think this one should be good, if you have any other thoughts
needs a rebase. also Circle still seems to be failing.
@chris-b1 this looks good. can you rebase once more. 

perf looks ok? (even if its slightly worse this is a bug :>)

Go ahead and merge on green.