Adding Absolute Tolerance Functionality #234
Conversation
marty-larocque
changed the title
matplotcheck/base.py
Outdated
| ``e``, and an actual value ``a``, this asserts | ||
| ``abs(a - e) < (e * 0.1)``. (This uses `np.testing.assert_allclose` | ||
| with ``rtol=tolerence`` and ``atol=inf``.) | ||
| tol_rel : float |
There was a problem hiding this comment.
do you think we need both absolute and relative? it seems like just picking one would keep things simpler. @nkorinek what do you think about this? we are addressing issues for when the data values are decimal places off but pretty much correct. a tolerance would account for that. i just am not sure we need two parameters for it. 1 seems like enough.
There was a problem hiding this comment.
let's remove relative tolerance and KEEP absolute tolerance which will work with values close to or at 0
matplotcheck/base.py
Outdated
| if xtime: | ||
| raise ValueError("tolerance must be 0 with datetime on x-axis") | ||
| np.testing.assert_allclose( | ||
| xy_data["x"], | ||
| xy_expected[xcol], | ||
| rtol=tolerence, | ||
| rtol=tol_rel, |
There was a problem hiding this comment.
@ryla5068 is the deal that the numpy allclose function has both parameters so you are exposing them in our method?
matplotcheck/base.py
Outdated
| @@ -909,19 +915,21 @@ def assert_xydata( | |||
| xy_expected.sort_values(by=xcol), | |||
| ) | |||
|
|
|||
| if tolerence > 0: | |||
| if tol_rel > 0 or tol_abs > 0: | |||
There was a problem hiding this comment.
once we figure this out it does seem like we should do a quick check to ensure the user provides a reasonable tolerance value. does assert_allclose check that the tolerance value is a number?
There was a problem hiding this comment.
a few changes here marty. Can you also please add a changelog update ? and then i just want to talk through relative vs absolute tolerance. i'm not sure we need both. Also what happens if the user provides both by mistake? it seems like picking one might be simplest but perhaps you had a reason for allowing for both?
|
oh also please update from master. this PR should then pass CI |
|
@nkorinek i pinged you on this pr as well. lets plan to focus on this package for the next week to get some of these issues and pr's resolved and merged and such! |
|
|
/rebase |
|
@lwasser This PR is failing when building docs. I'm not sure why, and it is not failing for me locally. Other than this, this PR should be ready to merge. |
|
thank you @nkorinek i will close this pr given it's opened and fixed in a new pr! |
This PR adds functionality for absolute tolerance in
base.assert_xydata()andbase.assert_bin_values(), addressing issue #157.