Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add names for test failures #1690

Closed
wants to merge 10 commits into from

Conversation

maaleske
Copy link
Contributor

@maaleske maaleske commented Nov 3, 2017

This PR adds the coordinate name to the test failure printout in assert_allclose() when cycling through the coordinates . Specifying the coordinate makes test failures much easier to diagnose just based on the message.

  • Closes #xxxx
  • Tests added / passed
  • Passes git diff upstream/master **/*py | flake8 --diff
  • Fully documented, including whats-new.rst for all changes and api.rst for new API

b.coords[v].values)
assert allclose, 'coord {}:{}\n{}'.format(a.coords[v].name,
a.coords[v].values,
b.coords[v].values)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding name to the error message for non-coordinates, too? We could do this by adding an optional name argument to this function. (It would also be useful for assert_equal and assert_identical.)

@maaleske maaleske changed the title Add coordinate names to assert_allclose Add names for test failures Nov 6, 2017
@maaleske
Copy link
Contributor Author

maaleske commented Nov 6, 2017

@shoyer I had to think a bit on your suggestion to figure out a way to make the name parameter work for the recursive cases, but now here's something to that effect. Also made me wonder how useful a test suite for testing test failures would be...

@@ -42,6 +42,8 @@ def assert_equal(a, b):
The first object to compare.
b : xarray.Dataset, xarray.DataArray or xarray.Variable
The second object to compare.
name : str, optional
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have a private function, e.g., assert_variable_equal, that accepts an optional name and call that from assert_equal? It's a little weird to allow names for Dataset objects.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't consider it as a name for either object, but rather an identifier for the specific assertion. Maybe it should be named/documented better as such?

@@ -27,7 +27,7 @@ def _data_allclose_or_equiv(arr1, arr2, rtol=1e-05, atol=1e-08,
arr1, arr2, rtol=rtol, atol=atol)


def assert_equal(a, b):
def assert_equal(a, b, name=''):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None is probably a better default value. It's valid (though obviously not a great idea) to have an xarray object with name given by an empty string.

@max-sixty
Copy link
Collaborator

Could this be merged? From what I can see, name is more like a message than related to the name of any object

@benbovy
Copy link
Member

benbovy commented Jan 18, 2019

Now that #1507 is merged, alternatively we could just reuse the diff_ formatting functions in assert_allclose() if the purpose here is to specify the coordinate or variable name in the failure report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants