-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 absolute tolerance when comparing FITS #4729
Conversation
Need a change log. Also... 👍 on more controls on the accuracy. However, this changes the API. I think usually we issue a deprecation warning first for a while, and then really change it. So might need that and a way for backward compatibility here, at least temporarily. I think this needs some further discussions between core maintainers. |
inserts magical @embray incantation |
This should keep |
"Affects-release" means "changes functionality in existing release" |
astropy/io/fits/diff.py
Outdated
|
||
abs(x1 - x2) > abstol + abs(x1)*reltol | ||
|
||
are considered to be different |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly nitpick but missing a period at the end here.
Thanks for the comments.
Is it ok to use |
Ops, I forgot to update the other |
@embray, sorry. 😅 I thought "affect release" means we will need to backport or something. I still can't find a documentation on how to properly use the "affect" tags. At one point I had to dig through a bunch of issues for that ASCII flowchart you made buried in the comments. But that's about it. Maybe for now, I'll just not touch those tags anymore... |
On one hand I usually would push against having more than one way to do it, and I'd be okay with deprecating As a compromise I think we could deprecate the |
astropy/io/fits/diff.py
Outdated
|
||
if tolerance is not None: | ||
warnings.warn("Named argument `tolerance` is deprecated " | ||
"and will be removed in future releases. Use `reltol` instead.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be formatted a bit difference. Grep the source for AstropyDeprecationWarning
for some other examples.
I like the approach that is taking shape including deprecating if tolerance is not None:
warnings.warn("Named argument `tolerance` is deprecated "
"and will be removed in future releases. Use `reltol` instead. "
"Ignoring `reltol` value...")
self.reltol = tolerance # when tolerance is provided *always* ignore `reltol`
# during the transition/deprecation period |
@embray , no one from SSB raised any objection thus far. So it's on them if they run into problem in the future. 👍 to this change. |
Numpy uses the convention |
@astrofrog I am for following numpy |
@elehcim - could you change |
@astrofrog I've changed tolerance names and implemented some comments noted on the discussion above. |
I actually liked @elehcim's original names over Numpy's--they're much clearer, and it's obvious enough how it maps onto the underlying Numpy function. But I'm not going to bikeshed any more than that. Decide amongst yourselves. |
This needs a rebase. And if the coverage decrease still shows up after, we need to figure out why. |
Rebased. Waiting for continuous integration checks. |
astropy/io/fits/diff.py
Outdated
@@ -352,7 +378,8 @@ def _report(self): | |||
wrapper.fill(ignore_fields))) | |||
self._writeln(u(' Maximum number of different data values to be ' | |||
'reported: {}').format(self.numdiffs)) | |||
self._writeln(u(' Data comparison level: {}').format(self.tolerance)) | |||
self._writeln(u(' Relative tolerance: {},' | |||
' Absolute tolerance: {}}').format(self.rtol, self.atol)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a superfluous }
here causing all kinds of testing failures
astropy/io/fits/diff.py
Outdated
@@ -262,6 +276,9 @@ def __init__(self, a, b, ignore_keywords=[], ignore_comments=[], | |||
ignore_blank_cards : bool, optional | |||
Ignore all cards that are blank, i.e. they only contain | |||
whitespace (default: True). | |||
|
|||
tolerance : float, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that's possible because the parameter was replaced by 2 new parameters. I haven't considered such a case. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MSeifert04 , I disagree. tolerance
is replacing rtol
, and atol
is a new feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But atol
is added in between existing parameters so it will permanently change the function signature. Say you call the function with positional parameters only then the old ignore_blanks
will now be interpreted as atol
.
But you're right for the tolerance
-> rtol
change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. That is too bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe not so "nice" but we could append "atol" at the end of the function signature. Then this is completly backwards-compatible. And of course using deprecated_renamed_argument
for the tolerance
-> rtol
change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think deprecated parameters should be added to the docstring. Probably best to remove it completly, the homepage contains the documentation for older versions and with the directive in rtol
about the renaming it should be fine, right?
Sphinx test failed on Travis CI; I don't see that failure on |
…rgument. Unfortunately it was not compatible with python2 decorator
@elehcim - I'm afraid you've added the changelog to the wrong section. This should go in to 2.0 and not 1.3.1 (as the latter is a bugfix release and that by definition doesn't contain enhancements or new deprecations). |
Ok, rebased and changed target version, thank you @bsipocz. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. My review is attached.
astropy/io/fits/diff.py
Outdated
|
||
.. math:: | ||
|
||
\\left| x_1 - x_2 \\right| > \\text{atol} + \\text{rtol} \\cdot \\left| x_1 \\right| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just following the formula from numpy.allclose is less confusing, since the keyword names are the same anyway.
astropy/io/fits/diff.py
Outdated
are considered to be different. | ||
|
||
atol : float, optional | ||
The allowed absolute difference. See also rtol parameter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Double backticks around rtol
.
astropy/io/fits/diff.py
Outdated
@@ -262,6 +274,9 @@ def __init__(self, a, b, ignore_keywords=[], ignore_comments=[], | |||
ignore_blank_cards : bool, optional | |||
Ignore all cards that are blank, i.e. they only contain | |||
whitespace (default: True). | |||
|
|||
tolerance : float, optional | |||
Alias of rtol. Deprecated, provided for backward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Double backticks around rtol
.
astropy/io/fits/diff.py
Outdated
self.rtol = rtol | ||
self.atol = atol | ||
|
||
if tolerance is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a in-code comment that this can be removed for v3.1 and after? Is that numbering right, @mhvk? Sorry, I am still learning the proper deprecated code removal policy.
@@ -43,7 +43,7 @@ | |||
Example | |||
------- | |||
|
|||
fitsdiff -k filename,filtnam1 -n 5 -d 1.e-6 test1.fits test2 | |||
fitsdiff -k filename,filtnam1 -n 5 -r 1.e-6 test1.fits test2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #5799.
@@ -252,7 +266,8 @@ def main(): | |||
opts.ignore_keywords = [] | |||
opts.ignore_comments = [] | |||
opts.ignore_fields = [] | |||
opts.tolerance = 0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, wait. Now I am confused. In the parser above, there is a dest="rtol"
for -d
option. But then, it is no handling for when both -d
and -r
are defined. And then below, somehow there is a tolerance=opts.tolerance
option. I think all these need some cleaning up.
diff = HeaderDiff(ha, hb, rtol=1e-5, atol=1e-5) | ||
assert diff.identical | ||
diff = HeaderDiff(ha, hb, atol=1.1e-5) | ||
assert diff.identical |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would like to see such a test as well, where both rtol
and tolerance
are defined, but the latter overrides the former.
astropy/io/fits/tests/test_diff.py
Outdated
assert diff.identical | ||
assert diff.diff_total == 0 | ||
|
||
def test_not_identical_within_rtol_and_atol(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function name implies that you are testing for "not identical" but then the actual test does the opposite. This is very confusing. Same comment for the test that follows below.
* handle the case of both `-r` and `-d` parameters passed to fitsdiff script
Hello @pllim, I think I managed to implement all your comments, thank you very much. |
I've restarted the failing builds. |
Two of them still failed with URLError. I restarted them again. |
🎉 travis did his job |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you added a test to the fitsdiff
command too, nice! Just a minor comment from me.
@MSeifert04 , do you want to look at this one last time?
astropy/io/fits/scripts/fitsdiff.py
Outdated
@@ -247,12 +261,25 @@ def main(): | |||
|
|||
opts, args = handle_options(argv) | |||
|
|||
if opts.tolerance is not None and opts.rtol is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the codes above, tolerance
overwrites rtol
with a warning, but here it is outright error. Why the difference in behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, maybe it's better to issue an AstropyDeprecationWarning
and let tolerance
overwrite rtol
from . import FitsTestCase | ||
from ..hdu import PrimaryHDU | ||
from ..scripts import fitsdiff | ||
from ....tests.helper import pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to @astrofrog , you'll need to change this too for #5694 if this gets merged first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I added a few additional comments and I'm not exactly sure the script is working as intended. Not because it seems wrong but just because I'm not familiar with scripts 😄
\\left| a - b \\right| > \\text{atol} + \\text{rtol} \\cdot \\left| b \\right| | ||
|
||
are considered to be different. | ||
The underlying function used for comparison is `numpy.allclose`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a .. versionchanged:: 2.0
-directive here explaining the change of parameter name tolerance->rtol
?
astropy/io/fits/diff.py
Outdated
@@ -262,6 +276,9 @@ def __init__(self, a, b, ignore_keywords=[], ignore_comments=[], | |||
ignore_blank_cards : bool, optional | |||
Ignore all cards that are blank, i.e. they only contain | |||
whitespace (default: True). | |||
|
|||
tolerance : float, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think deprecated parameters should be added to the docstring. Probably best to remove it completly, the homepage contains the documentation for older versions and with the directive in rtol
about the renaming it should be fine, right?
astropy/io/fits/diff.py
Outdated
self.atol = atol | ||
|
||
if tolerance is not None: # This should be removed in the next astropy version | ||
warnings.warn('"tolerance" was ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a PEP-8-nitpick but that seems like odd-indentation to me. Feel free to keep it as is but I would prefer:
warnings.warn(
'"tolerance" was deprecated in version 2.0 and will be removed in '
'a future version. Use argument "rtol" instead.',
AstropyDeprecationWarning)
because the hanging indentation is the same for all strings and parameters here. Same for the other occurances of this warning.
astropy/io/fits/diff.py
Outdated
""" | ||
|
||
if isinstance(a, float) and isinstance(b, float): | ||
if np.isnan(a) and np.isnan(b): | ||
return False | ||
return not np.allclose(a, b, tolerance, 0.0) | ||
return not np.allclose(a, b, rtol, atol) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you pass them in by name here as well? rtol=rtol, atol=atol
? That's what is done oion the other places as well.
… `fitsdiff` * Add versionchanged directive * Removed deprecated argument from docstring * Fix indentation
Thank you @MSeifert04 for your review. |
No specific doubts, it's just that these scripts are not covered by the test suite and I firmly believe that "untested code is broken code". 😄 (That doesn't mean that every case has to be tested but if there is no test at all then it may break down at any point). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
So, we got two approvals, but CircleCI failed. I am not familiar with CircleCI (the log wouldn't load for me), can someone please confirm whether failure is related or not?
@MSeifert04 , that sound a little extreme as it implies 100% coverage to be a requirement. 😅 |
I've restarted CircleCI. Untested code is broken code - anytime I increase coverage I find bugs! It's just a question of whether they are critical bugs :) |
That's hilarious! |
The PR started back in March 2016. 😅 Time to merge! Thank you for your patience, @elehcim ! |
This changes a bit the interface:
tolerance
->reltol
Added
abstol
keyword argument.This is particularly useful when numbers to compare are very little. I'm exploiting the underlying numpy function
numpy.allclose
which can be used also with absolute tolerance.