Deconvolving ringmap #122

jrs65 · 2021-02-03T16:52:07Z

No description provided.

lgtm-com · 2021-02-03T17:19:06Z

This pull request introduces 1 alert and fixes 1 when merging 47d5bd1 into 0c4b397 - view on LGTM.com

new alerts:

1 for Unused local variable

fixed alerts:

1 for Unnecessary pass

lgtm-com · 2021-02-03T21:54:15Z

This pull request introduces 1 alert and fixes 1 when merging 839bbf1 into 0c4b397 - view on LGTM.com

new alerts:

1 for Unused local variable

fixed alerts:

1 for Unnecessary pass

wulfda02 · 2021-02-15T17:01:52Z

Currently, you have the "natural" EW weights as [4,3,2,1], but I think it should be [2,3,2,1]. The 0-cylinder separation baselines don't include any independent negative v baselines, unlike the non-zero cylinder separations (thus reducing their weight by half). Right?

else:  # self.weight_ew == "natural"
            weight_ew = n_ew - np.arange(n_ew)

wulfda02 · 2021-02-15T17:10:50Z

Also, when forming the wiener filter, you are summing over the cylinder axis, as well as the +/-m axis in the denominator

C_inv = self.inv_SN + (mB.conj() * weight_ew[:, np.newaxis] * mB).sum(
                    axis=(1, -2)
                )

But, shouldn't we keep positive and negative m separate until after filtering? I.e. only sum over axis=-2

jrs65 · 2021-02-18T19:56:03Z

Currently, you have the "natural" EW weights as [4,3,2,1], but I think it should be [2,3,2,1]. The 0-cylinder separation baselines don't include any independent negative v baselines, unlike the non-zero cylinder separations (thus reducing their weight by half). Right?
else:  # self.weight_ew == "natural"
            weight_ew = n_ew - np.arange(n_ew)

This is a good question. I am pretty sure it's correct, but it's not trivial to come up with a concrete explanation as to why.

Here are a few different stabs at it:

With reference to how the standard ringmap maker works, the non-zero positive cylinder separations get implicitly conjugated into the negative separations and so by your argument you need to halve the weighting of all of them. This is relevant for the deconvolved ringmap maker because the standard ringmap maker is essentially the limit as you take the primary beam width to zero.
Second, as you point out there aren't independent negative baselines, so in some sense you should get half the weighting, but because of that same property there zero cylinder separation beams are purely real and thus have no noise in an imaginary part, so they have half the variance (i.e. twice the weight). These basically cancel out. This is one of the subtleties of interferometry, for very short baselines the noise properties are very different for real and imaginary parts. Interestingly this slightly odd effect disappears when using m-modes (in the general sense, I'm not sure about this specific hybrid case atm).

jrs65 · 2021-02-18T20:06:17Z

Also, when forming the wiener filter, you are summing over the cylinder axis, as well as the +/-m axis in the denominator
C_inv = self.inv_SN + (mB.conj() * weight_ew[:, np.newaxis] * mB).sum(
                    axis=(1, -2)
                )
But, shouldn't we keep positive and negative m separate until after filtering? I.e. only sum over axis=-2

In order to guarantee the output sky is real, you need to treat the positive/negative m's as separate measurements of the same underlying mode and only infer the m >= 0 modes (as the negative ones on the sky are simple conjugates). This is done in the m-mode papers, although I have a similar derivation of it for the hybrid case that I can tidy up. Ultimately it means that the +/- modes are independent measurements at a similar level to the different cylinders and thus you need to sum over both in the filter.

Ultimately I think the fact that the +/- modes are mostly disjoint (coming from above/below the NCP), means that this doesn't really matter a huge amount in the practical output for CHIME. However, any instrument that has an EW baseline length > 0, but < aperture width (i.e. a responses that covers both +/-m in some asymmetric manner) would need to account for this properly to get the right S/N weighting. CHIME gets away with it because either your baseline has + or - m response at a particular declination (but not both), or it is the 0-cyl separation in which case the response is symmetric for +/- m and they should get weighted equally.

jrs65 · 2021-02-19T02:37:24Z

I'm having doubts about the relative cylinder weighting point. I think I'm going to need to write it all out to be sure. I'll circulate it when I've got something written up.

lgtm-com · 2021-03-15T20:46:56Z

This pull request fixes 1 alert when merging 8c42861 into 0c4b397 - view on LGTM.com

fixed alerts:

1 for Unnecessary pass

ssiegelx · 2021-03-19T16:47:14Z

I'm finding that the maps generated with this task are scaled down by a factor of 4 relative to maps created with the standard ringmap maker. Also the flux of point sources is off by a factor of four from their expected value.

This is also true of the deconvolving map maker task that I wrote that uses an external beam model.

example_normalization_problem_3c295.pdf

This plot shows an RA slice through the map at the (RA, Dec) of 3C 295. I'm showing the standard ring map maker in blue (i.e., using the BeamformEW task on the same HybridVis container), this task in red, and my map maker in green. The expected flux of the source is indicated by the black dashed line.

@wulfda02 Have you encountered this?

wulfda02 · 2021-03-19T17:45:45Z

As I mentioned on the stacking call, I think this can be explained by the dirty beam amplitude after going through the wiener filter. It sounds like there's still some uncertainty as to why this amplitude isn't 1, but in my work I find that re-normalizing by this amplitude produces the correct flux for point sources.

ssiegelx · 2021-03-23T19:28:17Z

See PR #126 for an extension to this that allows for an external beam model.

jrs65 · 2021-04-19T22:13:22Z

@ssiegelx @wulfda02 as this doesn't effect any existing functionality (and you have your own updates to), I'd like to get this very quickly reviewed and merged into master, and then we can work on getting your changes merged on top.

anjakefala · 2021-04-20T00:07:24Z

draco/analysis/ringmapmaker.py

@@ -230,7 +231,7 @@ def process(self, gstream):
                vmax = nsmax / wv

            if self.weight == "inverse_variance":
-                gw = gsw[:, lfi].copy()
+                gw = gsw[:, lfi].view(np.ndarray).copy()


Should the view() occur before the slice?

Yeah, good point. This is fine with the current mpiarray code, but would result in a warning in the new one. Probably the right thing to do is to define gsw with a .local_array, but we'll need the new code to get merged first and I'm still a little reticent to do it atm.

I believe that both .local_array and moving the view before the slice is old-code compatible!

draco/analysis/transform.py

ssiegelx

@jrs65 I think I've identified one bug described in the comment above.

Beyond that, all of my suggestions are implemented in PR #126.

draco/analysis/ringmapmaker.py

anjakefala · 2021-04-20T01:17:09Z

draco/analysis/ringmapmaker.py

+                # Get the effective beamwidth for the polarisation combination
+                sig_a = beam_width[pa](freq, dec)
+                sig_b = beam_width[pb](freq, dec)
+                sig = sig_a * sig_b / (sig_a ** 2 + sig_b ** 2) ** 0.5


What is the order of operations that you expect here?

…sh` routine Due to some of the logic handling whether the method exists or not, it would mask and silence an AttributeError that was raised for other reasons in the actual `.process_finish` method when it ran.

Transform wasn't exactly invertible as mmax was incorrect.

lgtm-com · 2021-04-20T03:52:34Z

This pull request fixes 1 alert when merging 09a9fe1 into 63e68d4 - view on LGTM.com

fixed alerts:

1 for Unnecessary pass

jrs65 force-pushed the deconvolving-ringmap branch from 839bbf1 to 8c42861 Compare March 15, 2021 19:45

ssiegelx mentioned this pull request Mar 23, 2021

ss/deconvolving ringmap #126

Merged

anjakefala reviewed Apr 20, 2021

View reviewed changes

ssiegelx reviewed Apr 20, 2021

View reviewed changes

draco/analysis/transform.py Outdated Show resolved Hide resolved

ssiegelx approved these changes Apr 20, 2021

View reviewed changes

anjakefala reviewed Apr 20, 2021

View reviewed changes

draco/analysis/ringmapmaker.py Show resolved Hide resolved

anjakefala reviewed Apr 20, 2021

View reviewed changes

jrs65 added 4 commits April 19, 2021 20:22

fix(task): issue when an AttributeError is raised in a `.process_fini…

b245cfc

…sh` routine Due to some of the logic handling whether the method exists or not, it would mask and silence an AttributeError that was raised for other reasons in the actual `.process_finish` method when it ran.

feat(ringmapmaker): added a new task for deconvolving ringmaps

fb77790

style: apply black

cb6a5b5

fix(MModeTransform): incorrect mmax calculation

09a9fe1

Transform wasn't exactly invertible as mmax was incorrect.

jrs65 force-pushed the deconvolving-ringmap branch from 8c42861 to 09a9fe1 Compare April 20, 2021 03:22

jrs65 marked this pull request as ready for review April 20, 2021 03:27

jrs65 merged commit 46fa146 into master Apr 20, 2021

jrs65 deleted the deconvolving-ringmap branch April 20, 2021 03:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deconvolving ringmap #122

Deconvolving ringmap #122

jrs65 commented Feb 3, 2021

lgtm-com bot commented Feb 3, 2021

lgtm-com bot commented Feb 3, 2021

wulfda02 commented Feb 15, 2021

wulfda02 commented Feb 15, 2021

jrs65 commented Feb 18, 2021

jrs65 commented Feb 18, 2021

jrs65 commented Feb 19, 2021

lgtm-com bot commented Mar 15, 2021

ssiegelx commented Mar 19, 2021

wulfda02 commented Mar 19, 2021

ssiegelx commented Mar 23, 2021

jrs65 commented Apr 19, 2021

anjakefala Apr 20, 2021

jrs65 Apr 20, 2021

anjakefala Apr 20, 2021

ssiegelx left a comment

anjakefala Apr 20, 2021

jrs65 Apr 20, 2021

lgtm-com bot commented Apr 20, 2021

Deconvolving ringmap #122

Deconvolving ringmap #122

Conversation

jrs65 commented Feb 3, 2021

lgtm-com bot commented Feb 3, 2021

lgtm-com bot commented Feb 3, 2021

wulfda02 commented Feb 15, 2021

wulfda02 commented Feb 15, 2021

jrs65 commented Feb 18, 2021

jrs65 commented Feb 18, 2021

jrs65 commented Feb 19, 2021

lgtm-com bot commented Mar 15, 2021

ssiegelx commented Mar 19, 2021

wulfda02 commented Mar 19, 2021

ssiegelx commented Mar 23, 2021

jrs65 commented Apr 19, 2021

anjakefala Apr 20, 2021

Choose a reason for hiding this comment

jrs65 Apr 20, 2021

Choose a reason for hiding this comment

anjakefala Apr 20, 2021

Choose a reason for hiding this comment

ssiegelx left a comment

Choose a reason for hiding this comment

anjakefala Apr 20, 2021

Choose a reason for hiding this comment

jrs65 Apr 20, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Apr 20, 2021