Reintroduce Perley-Polyhedron Gridder #222

bennahugo · 2020-09-23T08:52:24Z

Reverts #220

This reverts commit ae8ae8e.

bennahugo · 2020-09-23T08:54:49Z

Need to try and undo the mess created by the revert. I will be basing off this branch to make the necessary workaround for the numba 0.5 bug

o-smirnov · 2020-09-23T09:02:07Z

Nothing like a revert of a revert to pervert one's brain!

sjperkins · 2020-09-23T09:02:39Z

@bennahugo Would you not prefer to work from #219 whose point was to retain the state of the PR prior to the revert?

sjperkins · 2020-09-23T09:43:29Z

Whatever works though, this has green lights on the test suite.

bennahugo · 2020-09-23T09:53:53Z

@sjperkins that has rather large changes with SanitaryLiterals. I think the issue is not in the use of SanitaryLiterals but in the way the new numba compiler handles literals. It seems you cannot create a stack of nested literals in a jit function called by another jit function. All literally calls must take place in the first jit function the compiler encounters on the stack, so I strongly suspect this is a compiler bug in numba >=0.5

sjperkins · 2020-09-23T09:54:29Z

@sjperkins that has rather large changes with SanitaryLiterals. I think the issue is not in the use of SanitaryLiterals but in the way the new numba compiler handles literals. It seems you cannot create a stack of nested literals in a jit function called by another jit function. All literally calls must take place in the first jit function the compiler encounters on the stack, so I strongly suspect this is a compiler bug in numba >=0.5

All good!

sjperkins · 2020-09-23T09:57:04Z

I think the numba version needs changing in the setup.py?

bennahugo · 2020-09-23T10:47:43Z

woops sorry you are right. I only upgraded locally

sjperkins · 2020-09-23T11:14:03Z

Well that was easy enough! Green lights. Let me give it a quick once over, unless there's more you wish to do?

bennahugo · 2020-09-23T11:18:57Z

Sure go ahead. The issue is simplified to the simplest possible reproducer that mimics this issue in numba/numba#6274

sjperkins · 2020-09-23T11:23:42Z

@bennahugo I'm a bit confused. Were you intending to post this on the related issue #219?

This is the PR reintroducing the gridder.

sjperkins · 2020-09-23T11:24:27Z

Happy to review though.

bennahugo · 2020-09-23T11:29:14Z

@bennahugo I'm a bit confused. Were you intending to post this on the related issue #219?

This is the PR reintroducing the gridder.

No, I just mentioned the post where I've reproduced the workaround for compiler bug that causes the failure on numba version 0.51.2 in a simpler case if you wanted to comment

sjperkins · 2020-09-23T11:33:49Z

@bennahugo I'm a bit confused. Were you intending to post this on the related issue #219?
This is the PR reintroducing the gridder.

No, I just mentioned the post where I've reproduced the workaround for compiler bug that causes the failure on numba version 0.51.2 in a simpler case if you wanted to comment

Ah! I'll take a look at that too.

sjperkins

Some minor nits, but otherwise this is good to go.

sjperkins · 2020-09-23T11:35:26Z

HISTORY.rst

@@ -6,6 +6,8 @@ History
 ------------------
 * Add wrapper for ducc0.wgridder (:pr:204`)
 * Correct Irregular Grid nesting in BeamAxes (:pr:`217`)
+* Add Perley Polyhedron Faceting Gridder/Degridder (:pr:`202`, :pr:`215`)


Could you start a new version section in the changelog that also mentions this PR? e.g.:

0.2.7 (YYYY-MM-DD) ---------------------------- * Add Perley Polyhedron Faceting Gridder/Degridder (:pr:`202`, :pr:`215`, :pr:`222`)

sjperkins · 2020-09-23T11:36:49Z

africanus/gridding/perleypolyhedron/kernels.py

+    with a modification of higher order bessels as default, as
+    this improves the kernel at low number of taps.
+    """
+    autocoeffs = np.polyfit(


Could the definition of this be moved outside the kbsinc function?

Don't think so it is specific for the function (the coefficents are not the same - see the original Jackson paper).

Ok, but you could have _kb_coeffs and _hanning_coeffs globals, for instance. They're constant and private by convention.

sjperkins · 2020-09-23T11:37:53Z

africanus/gridding/perleypolyhedron/kernels.py

+    autocoeffs = np.polyfit([1.5, 2.0, 2.5, 3.0, 3.5],
+                            [0.7600, 0.7146, 0.6185, 0.5534, 0.5185], 3)
+    if a is None:
+        a = np.poly1d(autocoeffs)((W + 2))


Similarly to kbsinc, could the autocoeffs be moved outside?

sjperkins · 2020-09-23T11:38:38Z

africanus/gridding/perleypolyhedron/kernels.py

+    return res / np.sum(res)
+
+
+def hanningsinc(W, a=0.5, oversample=5):


Shouldn't None be the default value for a?

Suggested change

def hanningsinc(W, a=0.5, oversample=5):

def hanningsinc(W, a=None, oversample=5):

well spotted yes will fix

sjperkins · 2020-09-23T11:41:05Z

africanus/gridding/perleypolyhedron/tests/test_daskintrf.py

+        nchan = 128
+        frequency = da.from_array(np.linspace(1.0e9, 1.4e9, nchan),
+                                  chunks=(nchan, ))
+        wavelength = 299792458.0 / frequency


It's possible to use the lightspeed definition from constants

from africanus.constants import c as lightspeed

sjperkins · 2020-09-23T11:42:02Z

africanus/gridding/perleypolyhedron/tests/test_daskintrf.py

+    pxacrossbeam = 10
+    nchan = 1024
+    frequency = np.array(np.linspace(1.0e9, 1.4e9, nchan))
+    wavelength = np.array([299792458.0 / f for f in frequency])


The np.array and list comprehensions aren't strictly necessary:

from africanus.constants import c as lightspeed frequency = np.linspace(1.0e9, 1.4e9, nchan) wavelength = lightspeed / frequency

sjperkins · 2020-09-23T11:44:22Z

africanus/gridding/perleypolyhedron/tests/test_daskintrf.py

+    pxacrossbeam = 10
+    nchan = 1024
+    frequency = np.array(np.linspace(1.0e9, 1.4e9, nchan))
+    wavelength = np.array([299792458.0 / f for f in frequency])


The np.array and list comprehensions aren't strictly necessary:

from africanus.constants import c as lightspeed frequency = np.linspace(1.0e9, 1.4e9, nchan) wavelength = lightspeed / frequency

sjperkins · 2020-09-23T11:44:30Z

africanus/gridding/perleypolyhedron/tests/test_daskintrf.py

+    pxacrossbeam = 10
+    nchan = 16
+    frequency = np.array(np.linspace(1.0e9, 1.4e9, nchan))
+    wavelength = np.array([299792458.0 / f for f in frequency])


The np.array and list comprehensions aren't strictly necessary:

from africanus.constants import c as lightspeed frequency = np.linspace(1.0e9, 1.4e9, nchan) wavelength = lightspeed / frequency

sjperkins · 2020-09-23T12:13:34Z

@bennahugo One final request: Could you rename the lambdas arguments to wavelengths in the gridder and degridder calls? I think its more explicit.

bennahugo · 2020-09-23T12:40:53Z

Fixes the issues raised apart from the coefficients which are function specific so I don't think they should be moved outside the function?? uvlambda is a common expression for stating uv measured in lambda - which is what was meant in the gridder interface. I've changed it though

sjperkins · 2020-09-23T13:23:23Z

Squashed into master. I'm very glad this is now in!

I'm aiming to push a release out today.

Revert "Revert perley polyhedron gridder (#220)"

c08bac6

This reverts commit ae8ae8e.

bennahugo added the wip Work in progress label Sep 23, 2020

bennahugo marked this pull request as draft September 23, 2020 08:55

Workaround for upstream bug in numba/numba#6274

62d0793

bump numba dependency version

c3dd06f

sjperkins self-requested a review September 23, 2020 11:33

bennahugo marked this pull request as ready for review September 23, 2020 11:42

sjperkins requested changes Sep 23, 2020

View reviewed changes

Fix issues raised in review

44a6d69

sjperkins added 3 commits September 23, 2020 14:52

Precompute kaiser-bessel and hanning coefficients as global constants

c16c514

Simplify band construction

5a5d7f5

flake8

e4d0895

sjperkins approved these changes Sep 23, 2020

View reviewed changes

sjperkins changed the title ~~Revert "Revert perley polyhedron gridder"~~ Reintroduce Perley-Polyhedron Gridder Sep 23, 2020

sjperkins merged commit 5224520 into master Sep 23, 2020

sjperkins deleted the revert-220-revert-perley-polyhedron-gridder branch September 23, 2020 13:18

sjperkins mentioned this pull request Sep 23, 2020

Perley Polyhedron Gridder literal interaction fails on Numba 0.51 #218

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce Perley-Polyhedron Gridder #222

Reintroduce Perley-Polyhedron Gridder #222

bennahugo commented Sep 23, 2020

bennahugo commented Sep 23, 2020

o-smirnov commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins left a comment

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

bennahugo Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

bennahugo Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

		return res / np.sum(res)


		def hanningsinc(W, a=0.5, oversample=5):

	def hanningsinc(W, a=0.5, oversample=5):
	def hanningsinc(W, a=None, oversample=5):

Reintroduce Perley-Polyhedron Gridder #222

Reintroduce Perley-Polyhedron Gridder #222

Conversation

bennahugo commented Sep 23, 2020

bennahugo commented Sep 23, 2020

o-smirnov commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020

sjperkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjperkins commented Sep 23, 2020

bennahugo commented Sep 23, 2020

sjperkins commented Sep 23, 2020