Fix convolve_fft nan_treatment='fill' handling by keflavich · Pull Request #8122 · astropy/astropy

keflavich · 2018-11-13T21:25:32Z

Fix for issue #8121

WIP: needs a test

regression test

astropy-bot · 2018-11-13T21:25:37Z

Check results are now reported in the status checks at the bottom of this page.

keflavich · 2018-11-13T21:29:29Z

It's not super clear to me how useful the nan_treatment='fill' mode is, but we'll keep it around. It probably makes sense to have a different fill_value for the edges and the NaNs.

pllim · 2018-11-13T21:39:23Z

@keflavich , ~~3.1.1 or 3.2?~~ nvm

bsipocz · 2018-11-13T21:39:41Z

I suppose this is a bugfix, but if it's not, please move to milestone 3.2

codecov · 2018-11-13T22:50:44Z

Codecov Report

Merging #8122 into master will increase coverage by 0.55%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #8122      +/-   ##
==========================================
+ Coverage   86.36%   86.91%   +0.55%     
==========================================
  Files         405      383      -22     
  Lines       60859    57859    -3000     
  Branches     1096     1056      -40     
==========================================
- Hits        52559    50291    -2268     
+ Misses       7676     6954     -722     
+ Partials      624      614      -10

Impacted Files	Coverage Δ
astropy/convolution/convolve.py	`85.41% <100%> (-1.11%)`	⬇️
astropy/convolution/__init__.py	`62.5% <0%> (-37.5%)`	⬇️
astropy/wcs/__init__.py	`50% <0%> (-16.67%)`	⬇️
astropy/visualization/wcsaxes/coordinates_map.py	`86.88% <0%> (-11.58%)`	⬇️
astropy/convolution/src/convolve.c	`80.68% <0%> (-8.34%)`	⬇️
astropy/wcs/src/unit_list_proxy.c	`53.96% <0%> (-6.62%)`	⬇️
astropy/visualization/units.py	`68.08% <0%> (-5.5%)`	⬇️
astropy/modeling/separable.py	`90.62% <0%> (-5.26%)`	⬇️
astropy/constants/__init__.py	`94.87% <0%> (-5.13%)`	⬇️
astropy/io/misc/asdf/tags/table/table.py	`90% <0%> (-4.81%)`	⬇️
... and 264 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ff9ac0...13694b1. Read the comment docs.

jamienoss · 2018-11-14T00:21:44Z

+                          AstropyUserWarning)
        rifft[arrayslices][nanmaskarray] = np.nan
+    elif nan_treatment == 'fill':
+        rifft[arrayslices][nanmaskarray] = fill_value


@keflavich I'm pretty confused. Why are the values being filled post convolution?

This needs to happen pre-convolution.

Good catch. This already happens pre-convolution, but not with the intended fill_value.

jamienoss · 2018-11-14T00:26:26Z


    if preserve_nan:
+        if nan_treatment == 'fill':
+            warnings.warn("preserve_nan was set, which overrides the "


This is not semantically true. Whilst preserve_nan=True will persist the NaN values that get filled, it doesn't/shouldn't "override" nan_treatment='fill' functionality. The values should be filled pre-convolution and therefore contribute to their surrounding convolutions regardless of whether they remain as the convolved values or revert to their original NaN values.

jamienoss

This doesn't implement nan_treatment='fill'. The values need to be filled pre-convolution.

jamienoss · 2018-11-14T00:30:30Z

+                          AstropyUserWarning)
        rifft[arrayslices][nanmaskarray] = np.nan
+    elif nan_treatment == 'fill':
+        rifft[arrayslices][nanmaskarray] = fill_value


This needs to happen pre-convolution.

keflavich · 2018-11-14T00:51:51Z

+    if nan_treatment == 'fill':
+        array[nanmaskarray] = fill_value
+    else:
+        array[nanmaskarray] = 0


@jamienoss this line replaced the values with the default fill_value of zero. So, the behavior was "always (kinda) correct" for fill_value=0, and we never had a test for other cases.

(this comment is the answer to #8122 (comment))

Ok, cool, thanks. It was right in front of me after all.

jamienoss · 2018-11-14T00:53:55Z

    else:
        rifft = ifftn(fftmult)

-    if preserve_nan:


I think this condition needs to stay.

keflavich · 2018-11-14T01:09:01Z

@jamienoss thanks for the review. There's a real chance I messed something up further; I clearly did not put in enough thought the first time around. It would be helpful to check that the tests are doing the right thing, i.e., coming up with the right numbers for all 3 elements in the array for the simple 1D convolutions.

jamienoss · 2018-11-14T03:00:10Z

@keflavich I'll take a look at the tests tomorrow.

jamienoss · 2018-11-14T18:43:27Z

-                              fill_value=np.nan)
+
+        with warnings.catch_warnings():
+            warnings.simplefilter('ignore', RuntimeWarning)


What's this for?

Prob needs a comment outlining its intent.

this is just a c&p from a previous test, so you're right.

I dropped the catching; it's not needed when filling.

jamienoss · 2018-11-14T19:01:02Z

+            warnings.simplefilter('ignore', RuntimeWarning)
+            result = convolve_fft(masked_array, kernel, boundary='fill',
+                                  nan_treatment='interpolate',
+                                  fill_value=np.nan)


Ok, so I've been trying to figure out the point to this test, specifically why fill_value=np.nan. In fact, I think it points out another bug. I'll open an issue.

In the meantime, what is the intent of this test and why fill_value=np.nan?

The original intent was to test that convolution could successfully ignore edge padding by setting the external pixels to NaN, I believe. However, that might be an outdated use case now that we have two different nan_treatment approaches.

jamienoss

I think there are a few issues lingering within the tests.

I tested them against convolve(), the results of the new tests are at least accurate.

jamienoss · 2018-11-14T19:03:05Z

+
        result = convolve_fft(masked_array, kernel, boundary='fill',
+                              nan_treatment='interpolate',
                              fill_value=np.nan)


Ok, so I've been trying to figure out the point to this test, specifically why fill_value=np.nan. In fact, I think it points out another bug. I'll open an issue.

In the meantime, what is the intent of this test and why fill_value=np.nan?

jamienoss · 2018-11-14T19:04:07Z

+        array = np.array([1., np.nan, 3.], dtype='float64')
+        kernel = np.array([1, 1, 1])
+
+        with warnings.catch_warnings():


What about this?

Right, again, in the fill cases, we shouldn't need to catch div-by-zero errors because there shouldn't be any. Fix incoming.

jamienoss · 2018-11-14T19:04:10Z

+        array = np.array([1., np.nan, 3.], dtype='float64')
+        kernel = np.array([1, 1, 1])
+
+        with warnings.catch_warnings():


jamienoss · 2018-11-14T19:04:39Z

+                                  fill_value=0)
+
+        # note that, because fill_value also affects boundary='fill', the edge
+        # pixels are treated as zero rather than being ignored.


Right, this is the bug, it shouldn't happen this way.

Yeah, this isn't so much a bug as a design failure, as highlighted in #8127

Probably, however, it's not always the case that the developer gets to define what is and isn't a bug. I.e. the artist of modern abstract art doesn't get to completely define its interpretation.

If the behaviour and functionality don't intuitively match the semantics (or the documentation (loophole)) then it is a bug. Even if the fix is just readjusting the semantics by documenting it as a feature (hence the loophole), it's still a bug until fixed.

I'm going to stick with "design failure" because neither the documentation nor the implementation actually make sense. As you pointed out, fill_value is incorrectly tied to both boundary and nan_treatement, which results in some nonsense parameter combinations.

That's said, this is all semantics, we have a bunch of crap that needs to be cleaned up, and I'm just arguing for punting the fixes related to this particular line to #8127, since it more accurately and completely describes the problem.

There is an open question here, though - should this PR be merged before dealing with the bigger issues in 8127, or should we try to get everything right all at once? I'm open to suggestions; I don't believe there are many/any users trying to use this corner of the convolution code yet, which gives us some flexibility, but we want it all to be right as soon as possible...

I'm cool to defer this particular point to #8127. I think it might be better and safer to address this piece by piece. I'm already trying to untangle the normalization and nan treatment as I did for convolve, so we already have a few parallel jobs.

👍 Thanks for doing this.

Just to give you some background: these problems mostly come from convolve and convolve_fft being developed fully independently (convolve by Tom, convolve_fft by me), then trying to match their call specifications. In trying to make convolve_fft match convolve's arguments more closely, I clearly missed/ignored several use cases that I had never considered. It's looking more likely that I caught all of the inverse, i.e., things convolve_fft did that needed to be ported over to convolve.

jamienoss · 2018-11-14T19:12:52Z

                                 nan_treatment=nan_treatment,
-                                 fill_value=np.nan if normalize_kernel else 0,
+                                 # you cannot fill w/nan, you can only interpolate over it
+                                 fill_value=np.nan if normalize_kernel and nan_treatment=='interpolate' else 0,


Even though this was already conditional on normalize_kernel, I don't think it should be constraining the test space. The addition of nan_treatment=='interpolate' also constrains the test and I don't think it is a good idea to do so. We should be trying to test all parameter space and just deal with the outcome appropriately in the test.

I've lost the thread of this conversation... are you saying we should run this test with both np.nan and 0 for fill values, and just check the returns for each case are correct? That's reasonable, and I think the expected outcome is:
if np.isnan(fill_value) and not (normalize_kernel and nan_treatment=='interpolate'): an exception should be raised, but all cases should be allowed with fill_value=0.

I'm not certain of this, though, I should go check....

jamienoss · 2018-11-14T19:13:43Z

            z = convolve_fft(x, y, boundary=boundary,
                             nan_treatment=nan_treatment,
-                             fill_value=np.nan if normalize_kernel else 0,
+                             # you cannot fill w/nan, you can only interpolate over it


jamienoss · 2018-11-14T19:19:15Z


 import itertools

+import warnings


When the other warnings are removed, don't forget to remove this also.

pllim · 2019-02-08T20:39:19Z

From the Astropy Coordination Meeting notes for 3.2 release, this is one of the items listed but looks like it is already milestoned as 3.1.x.

bsipocz · 2019-04-18T00:27:04Z

@keflavich - This seems to be very close to be finished. Do you have time to finalize it?

keflavich · 2019-04-18T01:10:37Z

@bsipocz it might be further from done than I realized...

bsipocz · 2019-04-18T01:47:05Z

@keflavich - sadly those test failures are coming from master. Anyway, if it's indeed further from done, please remilestone to 3.2.1

pllim · 2019-09-20T13:30:09Z

@keflavich , try a rebase.

there's still more to go

likely to break some tests)

pllim · 2019-10-24T16:41:58Z

@keflavich , change log needs fixing. FYI.

bsipocz · 2019-10-24T18:30:08Z

I'm happy to fix changelogs if they're the only leftover stuff in a PR (I need to do it anyway already).

bsipocz · 2019-10-24T18:32:25Z

@larrybradley @adonath - Could either of you please review and approve it if you're happy with it?

larrybradley · 2019-10-28T17:48:56Z

@keflavich Is this PR ready for final review. It looks like there are several unresolved conversations and your last comment of "I don't know the current state" suggests to me that this may need more work? If that's the case, I'll re-milestone for 4.1.

keflavich · 2019-10-28T17:51:32Z

@larrybradley It needs a review. I don't believe it needs more work, though.

larrybradley

Thanks, @keflavich.

fix: #8121

larrybradley · 2019-10-28T18:32:22Z

@bsipocz This needs a new changelog entry.

bsipocz · 2019-10-28T18:33:37Z

This needs a new changelog entry.

is it a bugfix or a feature? I could argue for either of those...

larrybradley · 2019-10-28T18:40:00Z

I'd call it a bugfix. The nan_treatment docs stated that fill_value would be used to fill NaN, but it wasn't doing that. Instead, the fill value for NaNs was always set to 0.

keflavich force-pushed the issue8121 branch 3 times, most recently from 2005274 to c36af00 Compare November 13, 2018 21:33

pllim added the convolution label Nov 13, 2018

bsipocz added this to the 3.1.1 milestone Nov 13, 2018

jamienoss reviewed Nov 14, 2018

View reviewed changes

jamienoss suggested changes Nov 14, 2018

View reviewed changes

keflavich commented Nov 14, 2018

View reviewed changes

jamienoss reviewed Nov 14, 2018

View reviewed changes

bsipocz modified the milestones: 3.1.1, 3.1.2 Dec 31, 2018

pllim modified the milestones: 3.1.2, v3.1.3 Feb 20, 2019

keflavich force-pushed the issue8121 branch from 14cede3 to e836462 Compare April 18, 2019 01:11

bsipocz removed this from the v3.1.3 milestone Apr 19, 2019

keflavich added 13 commits September 20, 2019 17:22

fix for issue 8121

aa1b292

initial work on a test. The test is wrong and fails right now.

8fbdfe1

worked through some tests and fixed obvious errors in the main code, but

68c8b0e

there's still more to go

fix tests

4eee8ab

change the filled value to fill_value pre-convolution (this is *very*

8777711

likely to break some tests)

remove post-convolution filling

6a279df

add another regression test with non-zero fill_value

0e74888

revert bad removal of preserve_nan

94b70fe

work toward fixing tests

0e60af0

tests pass again... hope they're saner this time

da94d6e

don't catch runtimewarnings for fill test, it's not needed

2858553

remove more unnecessary warning catches

60f7a16

remove unneeded import

13694b1

keflavich force-pushed the issue8121 branch from e836462 to 13694b1 Compare September 20, 2019 16:22

bsipocz requested review from adonath and larrybradley October 24, 2019 18:31

larrybradley approved these changes Oct 28, 2019

View reviewed changes

larrybradley merged commit df61912 into astropy:master Oct 28, 2019

larrybradley mentioned this pull request Oct 28, 2019

Is convolution.convolve_fft(nan_treatment='fill') implemented? #8121

Closed

astrofrog added no-changelog-entry-needed and removed no-changelog-entry-needed labels May 19, 2020

Conversation

keflavich commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astropy-bot Bot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keflavich commented Nov 13, 2018

Uh oh!

pllim commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bsipocz commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Nov 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jamienoss Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jamienoss Nov 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamienoss 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!

keflavich commented Nov 14, 2018

Uh oh!

jamienoss commented Nov 14, 2018

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!

jamienoss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

keflavich commented Nov 13, 2018 •

edited

Loading

astropy-bot Bot commented Nov 13, 2018 •

edited

Loading

pllim commented Nov 13, 2018 •

edited

Loading

bsipocz commented Nov 13, 2018 •

edited

Loading

codecov Bot commented Nov 13, 2018 •

edited

Loading

jamienoss Nov 14, 2018 •

edited

Loading

jamienoss Nov 14, 2018 •

edited

Loading

jamienoss left a comment •

edited

Loading