Skip to content
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

Refactor delay-space measurement code and add features #217

Merged
merged 12 commits into from
Aug 1, 2023

Conversation

sjforeman
Copy link
Contributor

This is meant to supersede #176 and #216:

Notably, I've kept the scheme of compressing many axes into a baseline axis. I don't think this is ideal in the long run, but in the short term, I think it's higher-priority to get the Wiener filter and #216 features all merged coherently

Copy link
Contributor

@ljgray ljgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I have a couple of nitpicky stylistic comments but that's about it

draco/core/containers.py Outdated Show resolved Hide resolved
draco/analysis/delay.py Show resolved Hide resolved
draco/analysis/delay.py Outdated Show resolved Hide resolved
draco/analysis/delay.py Outdated Show resolved Hide resolved
@sjforeman sjforeman force-pushed the sf/delay-refactor branch 3 times, most recently from 72178c4 to 99fc7bc Compare March 17, 2023 23:03
@sjforeman sjforeman requested a review from ljgray March 17, 2023 23:11
Copy link
Contributor

@ljgray ljgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me overall. I spoke to @jrs65 on Friday and he mentioned having some thoughts about modifying the delay spectrum implementation to be split into multiple pipeline tasks. So I don't have anything else to change here, but we should let him weight in as well.

@Arnab-half-blood-prince
Copy link
Contributor

Arnab-half-blood-prince commented May 9, 2023

Hi, I am also checking this and it looks good to me. However, I have one suggestion to add the freq axis in the index_map of the DelaySpectrum and DelayTransform container. This is needed to estimate various cosmological quantities downstream. For example, we need redshift to estimate comoving distances to get the kperp and kpar axes.

Probably something like this within the task -

delay_spec.create_index_map("freq",ss.freq)

And add this in the container -

@property
def freq(self):
    return self.index_map["freq"]

Arnab Chakraborty and others added 11 commits July 28, 2023 14:32
…ay_filter): use window functions from draco.util.tools
…oise covariance

The previous code version took the inverse noise variance for each
complex-valued measurement in frequency space, and assigned 2**-0.5
times this variance to the real and imaginary parts. However, for a
circularly-symmetric complex Gaussian random number, the variances
of the real and imaginary parts should each be 0.5 times the total
variance, implying that the separate inverse variances should be
2 times the total inverse variance.

The only impact of this error on previous output of the code is that
the noise level assumed by the Gibbs sampler was slightly wrong.
…rumWienerEstimator): add parameter to boost noise weights
To distinguish between routines that deal with delay spectra
instead of power spectra, we rename delay_spectrum_gibbs
to delay_power_spectrum_gibbs, and also alias delay_spectrum_gibbs
to the new name to preserve backwards compatibility.
@sjforeman sjforeman merged commit 79f24cf into master Aug 1, 2023
4 checks passed
@sjforeman sjforeman deleted the sf/delay-refactor branch August 1, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants