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 option to pass callable assertion failure message generator #5607

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

naught101
Copy link

It is nice to be able to write custom assertion error messages on failure sometimes. This allows that with the array comparison assertions, by allowing a fail_func(a, b) callable to be passed in to each assertion function.

Not tested yet, but I'm happy to add tests if this is something that would be appreciated.

  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@TomNicholas
Copy link
Member

Thanks for this suggestion @naught101 - can you give an example use case for this within the xarray codebase? Or are you just making a general suggestion of something we might find useful?

@keewis
Copy link
Collaborator

keewis commented Jul 15, 2021

assert_equal and assert_identical are currently calling .equals / .identical and the only thing they really add on top of that is the diff formatting, which is what you want to override. So for those at least it might be better to write your own assert_* functions.

assert_allclose, however, implements functionality that is otherwise unavailable (I think?) so we might want to think about exposing that separately and calling it in assert_allclose.

@max-sixty
Copy link
Collaborator

(if we do this we should make it kwarg-only)

@naught101
Copy link
Author

naught101 commented Jul 16, 2021

@TomNicholas My particular use case is that have datasets that are large enough that I can't see the full diff, so might miss major changes. I'm wanting to pass in something like lambda a, b: f"Largest difference in data is {abs(a-b).max().item()}", so I can quickly see if the changes are meaningful. Obviously a more complex function might also be useful, like a summary/describe table output of the differences..

I know I could set the tolerances higher, but the changes are not numerical errors, and I want to see them before updating the test data that they are comparing against.

Entirely possible that there are better ways to do this, of course :)

@max-sixty
Copy link
Collaborator

My particular use case is that have datasets that are large enough that I can't see the full diff, so might miss major changes. I'm wanting to pass in something like lambda a, b: f"Largest difference in data is {abs(a-b).max().item()}", so I can quickly see if the changes are meaningful. Obviously a more complex function might also be useful, like a summary/describe table output of the differences..

Whether or not we merge this, I would be a +1 on improving the output of the canonical function better, such that we can see differences easily. I would also imagine we can iterate on those without worrying about backward-compat, given this is generating output for people.

As an example, printing the result of the exact statement above for large arrays, maybe also with dimension values, would be great!

@shoyer
Copy link
Member

shoyer commented Jul 16, 2021

I think the more standard way to handle this is to add an argument for supplying an auxiliary err_msg rather than a callback. But this would definitely be welcome functionality!

@naught101
Copy link
Author

@shoyer That would either not work, or be needlessly expensive, I think. The message generation might be expensive (e.g. if I want a sum or mean of the differences). With a call back it only happens if it is needed. With a pre-computed message it would be computed every time.. Correct me if I'm wrong.

@github-actions
Copy link
Contributor

Unit Test Results

         6 files           6 suites   53m 43s ⏱️
16 172 tests 14 421 ✔️ 1 731 💤   20 ❌
90 228 runs  81 957 ✔️ 8 151 💤 120 ❌

For more details on these failures, see this check.

Results for commit fe39e36.

@max-sixty
Copy link
Collaborator

@naught101 to what extent do you think your wants are idiosyncratic vs. better overall? To the extent this is for things that would make the function better for all, then one option is to implement them; e.g. add f"Largest difference in data is {abs(a-b).max().item()}"`.

Possibly that reduces the opportunity to experiment though? People could probably monkey-patch the function in their own environments if they want. And I think we can also be fairly liberal about merging changes given the output is read by people.

@shoyer
Copy link
Member

shoyer commented Jul 17, 2021

@shoyer That would either not work, or be needlessly expensive, I think. The message generation might be expensive (e.g. if I want a sum or mean of the differences). With a call back it only happens if it is needed. With a pre-computed message it would be computed every time.. Correct me if I'm wrong.

I'm mostly thinking of the precedent from the standard library's unittest library and numpy.testing.

It does indeed add a bit of expense to add a custom message, but I guess it's generally acceptable for testing? If not, I would kind of expect to add support for callables in those libraries first.

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.

6 participants