Add absolute tolerance when comparing FITS by elehcim · Pull Request #4729 · astropy/astropy

elehcim · 2016-03-24T12:24:12Z

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.

pllim · 2016-03-24T15:31:33Z

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.

pllim · 2016-03-24T15:32:16Z

inserts magical @embray incantation

embray · 2016-03-25T09:33:02Z

This should keep tolerance as an alias for the existing functionality, but I otherwise agree that where needed it's good to have separate absolute and relative tolerances passed through.

embray · 2016-03-25T09:33:31Z

"Affects-release" means "changes functionality in existing release"

embray · 2016-03-25T09:34:42Z

+
+                abs(x1 - x2) > abstol + abs(x1)*reltol
+
+            are considered to be different


Silly nitpick but missing a period at the end here.

elehcim · 2016-03-25T12:58:33Z

Thanks for the comments.
I'd propose something like the following:

def __init__(self, a, b, ignore_keywords=[], ignore_comments=[],
             ignore_fields=[], numdiffs=10, reltol=0.0, abstol=0.0,
             ignore_blanks=True, ignore_blank_cards=True, tolerance=None):

    [...]
    self.reltol = reltol
    self.abstol = abstol
    if tolerance is not None:
        warn("Named argument `tolerance` is deprecated and will be removed in future releases.
              Use `reltol` instead.")
        if reltol == 0.0:
            self.reltol = tolerance

Is it ok to use reltol in case FITSDiff(reltol=1e-2, tolerance=1e-3)?
Whats your standard way to warn the user of a deprecated syntax?
I'm pushing some more code, you can comment on that.

elehcim · 2016-03-25T13:04:50Z

Ops, I forgot to update the other __init__ methods 😅. Before I proceed, please comment on the idea.

pllim · 2016-03-25T16:40:47Z

@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...

embray · 2016-03-29T16:18:43Z

On one hand I usually would push against having more than one way to do it, and I'd be okay with deprecating tolerance. On the other hand, I know there are enough tests at STScI using this that I don't want to be cropping up ~~from beyond the grave 👻~~ to cause hair-pulling and headaches over that.

As a compromise I think we could deprecate the tolerance keyword argument to FITSDiff (since I think the API is used less frequently, but we might want to check on that--@pllim?) but keep the existing options in the command-line arguments (without a deprecation warning).

embray · 2016-03-29T16:19:19Z

+
+        if tolerance is not None:
+            warnings.warn("Named argument `tolerance` is deprecated "
+                "and will be removed in future releases. Use `reltol` instead.")


This should be formatted a bit difference. Grep the source for AstropyDeprecationWarning for some other examples.

mcara · 2016-03-29T17:32:34Z

I like the approach that is taking shape including deprecating tolerance parameter. Except, for a little bit more clarity, I would suggest the following change to the logic [the confusion may arise from the fact that in the current implementation tolerance is sometimes used and sometimes ignored]:

    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

pllim · 2016-04-18T16:16:28Z

I know there are enough tests at STScI using this that...

@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.

astrofrog · 2016-05-10T10:39:56Z

Numpy uses the convention atol and rtol instead of abstol and reltol - does anyone have any thoughts on whether to follow this?

mcara · 2016-05-10T11:54:49Z

@astrofrog I am for following numpy

astrofrog · 2016-05-12T23:10:34Z

@elehcim - could you change abstol to atol and reltol to rtol? (if you can make this change by the end of tomorrow, we can include this in Astropy 1.2)

elehcim · 2016-05-17T13:44:43Z

@astrofrog I've changed tolerance names and implemented some comments noted on the discussion above.

embray · 2016-05-17T15:55:29Z

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.

pllim · 2016-12-02T19:44:31Z

This needs a rebase. And if the coverage decrease still shows up after, we need to figure out why.

elehcim · 2016-12-03T18:02:16Z

Rebased. Waiting for continuous integration checks.

bsipocz · 2016-12-03T19:53:12Z

                        '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 is a superfluous } here causing all kinds of testing failures

pllim · 2016-12-05T15:22:01Z

            Ignore all cards that are blank, i.e. they only contain
            whitespace (default: True).
+
+        tolerance : float, optional


The deprecation of tolerance should now follow the same implementation similar to #5171, utilizing a new decorator added in #5214.

I don't think that's possible because the parameter was replaced by 2 new parameters. I haven't considered such a case. 😅

@MSeifert04 , I disagree. tolerance is replacing rtol, and atol is a new feature.

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.

Hmm. That is too bad.

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.

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?

pllim · 2016-12-05T16:28:51Z

Sphinx test failed on Travis CI; I don't see that failure on master. I cancelled the Mac build.

…rgument. Unfortunately it was not compatible with python2 decorator

bsipocz · 2017-02-12T21:11:04Z

@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).

elehcim · 2017-02-13T13:34:42Z

Ok, rebased and changed target version, thank you @bsipocz.
Reviewers, please have a look at it. @pllim, @MSeifert04

pllim

Thanks for the updates. My review is attached.

pllim · 2017-02-13T16:37:32Z

+
+            .. math::
+
+                \\left| x_1 - x_2 \\right| > \\text{atol} + \\text{rtol} \\cdot \\left| x_1 \\right|


I think just following the formula from numpy.allclose is less confusing, since the keyword names are the same anyway.

pllim · 2017-02-13T16:37:49Z

+            are considered to be different.
+
+        atol : float, optional
+            The allowed absolute difference. See also rtol parameter.


Nitpick: Double backticks around rtol.

pllim · 2017-02-13T16:38:30Z

            whitespace (default: True).
+
+        tolerance : float, optional
+            Alias of rtol. Deprecated, provided for backward compatibility.


Nitpick: Double backticks around rtol.

pllim · 2017-02-13T16:39:45Z

+        self.rtol = rtol
+        self.atol = atol
+
+        if tolerance is not None:


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.

pllim · 2017-02-13T16:42:52Z

 -------

-    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


pllim · 2017-02-13T16:46:07Z

        opts.ignore_comments = []
        opts.ignore_fields = []
-        opts.tolerance = 0.0
+        opts.rtol = 0.0


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.

pllim · 2017-02-13T16:51:13Z

+        diff = HeaderDiff(ha, hb, rtol=1e-5, atol=1e-5)
+        assert diff.identical
+        diff = HeaderDiff(ha, hb, atol=1.1e-5)
+        assert diff.identical


Yes, I would like to see such a test as well, where both rtol and tolerance are defined, but the latter overrides the former.

pllim · 2017-02-13T16:54:16Z

+        assert diff.identical
+        assert diff.diff_total == 0
+
+    def test_not_identical_within_rtol_and_atol(self):


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.

…ix tests.

* handle the case of both `-r` and `-d` parameters passed to fitsdiff script

elehcim · 2017-02-15T09:45:18Z

Hello @pllim, I think I managed to implement all your comments, thank you very much.
I cannot see the link between the travis failures and this PR... do you have any idea?

bsipocz · 2017-02-15T10:04:02Z

I've restarted the failing builds.

pllim · 2017-02-15T14:44:28Z

Two of them still failed with URLError. I restarted them again.

elehcim · 2017-02-15T16:27:39Z

🎉 travis did his job

pllim

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?

pllim · 2017-02-15T20:35:54Z

    opts, args = handle_options(argv)

+    if opts.tolerance is not None and opts.rtol is not None:
+        sys.stderr.write(


In the codes above, tolerance overwrites rtol with a warning, but here it is outright error. Why the difference in behavior?

Yeah, maybe it's better to issue an AstropyDeprecationWarning and let tolerance overwrite rtol

pllim · 2017-02-15T20:40:37Z

+from . import FitsTestCase
+from ..hdu import PrimaryHDU
+from ..scripts import fitsdiff
+from ....tests.helper import pytest


Note to @astrofrog , you'll need to change this too for #5694 if this gets merged first.

MSeifert04

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 😄

MSeifert04 · 2017-02-16T11:48:18Z

+                \\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`.


Could you add a .. versionchanged:: 2.0-directive here explaining the change of parameter name tolerance->rtol?

MSeifert04 · 2017-02-16T11:49:49Z

            Ignore all cards that are blank, i.e. they only contain
            whitespace (default: True).
+
+        tolerance : float, optional


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?

MSeifert04 · 2017-02-16T11:54:34Z

+        self.atol = atol
+
+        if tolerance is not None:  # This should be removed in the next astropy version
+            warnings.warn('"tolerance" was '


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.

MSeifert04 · 2017-02-16T11:56:43Z

        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)


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

elehcim · 2017-02-16T16:27:14Z

Thank you @MSeifert04 for your review.
What are your doubts regarding the script?

MSeifert04 · 2017-02-16T19:06:49Z

What are your doubts regarding the script?

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).

MSeifert04

👍

pllim · 2017-02-17T17:46:24Z

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?

untested code is broken code

@MSeifert04 , that sound a little extreme as it implies 100% coverage to be a requirement. 😅

astrofrog · 2017-02-17T17:50:41Z

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 :)

mcara · 2017-02-17T17:59:34Z

anytime I increase coverage I find bugs!

That's hilarious!

pllim · 2017-02-17T18:05:23Z

The PR started back in March 2016. 😅 Time to merge! Thank you for your patience, @elehcim !

pllim added io.fits Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 24, 2016

embray added Affects-release and removed Affects-dev PRs and issues that do not impact an existing Astropy release labels Mar 25, 2016

embray added the Enhancement label Mar 25, 2016

embray reviewed Mar 25, 2016
View reviewed changes

embray reviewed Mar 29, 2016
View reviewed changes

astrofrog added the Ready-for-final-review label May 10, 2016

bsipocz reviewed Dec 3, 2016

View reviewed changes

pllim reviewed Dec 5, 2016

View reviewed changes

elehcim added 3 commits February 12, 2017 19:43

Add absolute tolerance when comparing FITS

9cb8796

Revert use of deprecated_renamed_argument decorator for tolerance a…

218f4c3

…rgument. Unfortunately it was not compatible with python2 decorator

Fix CHANGES.rst and version in deprecation warning

af9b709

elehcim mentioned this pull request Feb 12, 2017

Add tests for scripts in io.fits #5799

Closed

3 tasks done

bsipocz added this to the v2.0.0 milestone Feb 12, 2017

elehcim added 2 commits February 12, 2017 23:47

Change target version to 2.0

3491279

Fix escaped expression in formula

338cc0a

pllim requested changes Feb 13, 2017

View reviewed changes

elehcim added 4 commits February 14, 2017 11:53

Fix formula for rtol and atol using numpy.allclose as reference and f…

63f1626

…ix tests.

Add fitsdiff tests

830d3cc

* handle the case of both `-r` and `-d` parameters passed to fitsdiff script

Fix rtol test and skip wildcard test

b77a657

check at least error code when testing wildcard

ebbc4a8

pllim approved these changes Feb 15, 2017

View reviewed changes

MSeifert04 reviewed Feb 16, 2017

View reviewed changes

Issue a warning instead of an error when using deprecated argument in…

7cc1f6f

… `fitsdiff` * Add versionchanged directive * Removed deprecated argument from docstring * Fix indentation

MSeifert04 approved these changes Feb 16, 2017

View reviewed changes

pllim merged commit 5b05ffb into astropy:master Feb 17, 2017

mcara mentioned this pull request Apr 6, 2017

Replace deprecated option '-d' in 'fitsdiff' with '-r' spacetelescope/pandokia#3

Merged


		abs(x1 - x2) > abstol + abs(x1)*reltol

		are considered to be different


		.. math::

		\\left\| x_1 - x_2 \\right\| > \\text{atol} + \\text{rtol} \\cdot \\left\| x_1 \\right\|

Conversation

elehcim commented Mar 24, 2016

Uh oh!

pllim commented Mar 24, 2016

Uh oh!

pllim commented Mar 24, 2016

Uh oh!

embray commented Mar 25, 2016

Uh oh!

embray commented Mar 25, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elehcim commented Mar 25, 2016

Uh oh!

elehcim commented Mar 25, 2016

Uh oh!

pllim commented Mar 25, 2016

Uh oh!

embray commented Mar 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcara commented Mar 29, 2016

Uh oh!

pllim commented Apr 18, 2016

Uh oh!

astrofrog commented May 10, 2016

Uh oh!

mcara commented May 10, 2016

Uh oh!

astrofrog commented May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elehcim commented May 17, 2016

Uh oh!

embray commented May 17, 2016

Uh oh!

pllim commented Dec 2, 2016

Uh oh!

elehcim commented Dec 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Dec 5, 2016

Uh oh!

bsipocz commented Feb 12, 2017

Uh oh!

elehcim commented Feb 13, 2017

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

astrofrog commented May 12, 2016 •

edited

Loading

elehcim commented Feb 15, 2017 •

edited

Loading