BUG: ma.testutils.assert_equal on nans throws AssertionError #6661

[mcdaniel67]

Fixes #6661 and makes np.ma.testutils.assert_equal's functionality more closely match np.testing.assert_equal

Previous Functionality:

>>> import numpy as np
>>> from numpy.ma.testutils import assert_equal
>>> from numpy.testing import assert_equal as a_e
>>> a_e(np.nan, np.nan)
>>> assert_equal(np.nan, np.nan)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "numpy/ma/testutils.py", line 130, in assert_equal
    raise AssertionError(msg)
AssertionError:
Items are not equal:
 ACTUAL: nan
 DESIRED: nan

Modified Functionality:

>>> import numpy as np
>>> from numpy.ma.testutils import assert_equal
>>> from numpy.testing import assert_equal as a_e
>>> a_e(np.nan, np.nan)
>>> assert_equal(np.nan, np.nan)

[charris]

Use (...) instead of line continuation here.

[charris]

Oh, this is a horrible, horrible function. Let me think about it...

[mcdaniel67]

Ok, either way that should be a minor change to make. The reason I elected to go for the line continuation was matching the style used in line 137. And yeah, this function is... interesting.

Would it be worth adding unit tests for np.ma.testutils.assert_equal and working on refactoring?

[charris]

Note xor with two booleans is !=, so that simplifies some stuff also.

Yes, a test would be highly desirable, required even ;)

[charris]

Here is equivalent

def assert_equal(actual, desired, err_msg=''):
    if not (isinstance(actual, masked_array) or
            isinstance(desired, masked_array)):
        return utils.assert_equal(actual, desired)
    if isinstance(desired, dict):
        raise AssertionError()
    if (actual is masked) != (desired is masked):
        raise ValueError()

    actual = np.asanyarray(actual)
    desired = np.asanyarray(desired)
    if actual.dtype.kind == "S" and desired.dtype.kind == "S":
        actual = actual.tolist()
        desired = actual.tolist()
        return _assert_equal_on_sequences(actual, desired, err_msg=err_msg)
    return assert_array_equal(actual, desired, err_msg)

Note that unicode strings are not handled (python 3) and I'm not clear on why the special casing unless there is (was) a problem with fill value. Should be able to use an empty string for masked fill values I think, but currently maskled string arrays use "N/A". We should consider changing that.

[mcdaniel67]

Ok, thanks! Should I use that as a refactored base to make my changes on top of?

[charris]

It needs some filling out with error messages and such, it is a stripped down version. I think it should solve your problem as is, delegating it to the unmasked version. Writing some tests first would be good. The string problem needs an enhancement. I think we should be using an empty string in those cases.

[charris]

I think the strings can be handled like so. sixu needs import from numpy.compat.

def assert_equal(actual, desired, err_msg=''):
    # if nothing masked, default to ndarray version.
    if not (isinstance(actual, masked_array) or
            isinstance(desired, masked_array)):
        return utils.assert_equal(actual, desired)

    # if desired is a dict, actual must be also.
    if isinstance(desired, dict):
        raise AssertionError()
    # masked only compares with masked.
    if (actual is masked) != (desired is masked):
        raise ValueError()

    actual = np.asanyarray(actual)
    desired = np.asanyarray(desired)

    # Deal with string types
    if actual.dtype.kind in "SU":
        val = b'' if actual.dtype.kind == "S" else sixu('')
        actual = masked_array(actual, copy=False, subok=True, fill_value=val)
    if desired.dtype.kind in "SU":
        val = b'' if desired.dtype.kind == "S" else sixu('')
        desired = masked_array(desired, copy=False, subok=True, fill_value=val)

    return assert_array_equal(actual, desired, err_msg)

[mcdaniel67]

Ah alright, perfect! Thanks for the help, I'll try my hand at implementing the described changes!

[mcdaniel67]

Should I push my commits directly to this branch, or would the refactoring indicate that I should do this as a separate PR?

[charris]

Just push to your branch. When everything is ready, you can squash your commits together and rewrite the first commit message to match format in http://docs.scipy.org/doc/numpy-1.10.1/dev/gitwash/development_workflow.html, followed by a force push git push -f origin <your branch>.

[mcdaniel67]

@charris Could you re-review this? I think I've gotten it into a pretty good state.

[mcdaniel67]

@charris Should I recreate this PR or will it still be reviewed? I know it's far enough down in the listed PRs now that it has the potential to be skipped over easily.

[charris]

Why is this needed?

[charris]

I'll need to take a closer look, I note that you did things differently than suggested, so I need to restudy the problem.

[charris]

I don't think this should be a separate function, it makes the code harder to follow and is only used once.

[charris]

Close and reopen to see if tests of unsupported Python versions go away. Otherwise may need a rebase on master

[charris]

@mcdaniel67 Needs to be finished.

[homu]

☔ The latest upstream changes (presumably #7099) made this pull request unmergeable. Please resolve the merge conflicts.