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

Add QQ, UU, QU indexing to Fourier-diagonal EB covariances #22

Merged
merged 2 commits into from
Sep 26, 2020

Conversation

cailmdaley
Copy link
Collaborator

This PR adds functionality to rotate EB covariances into QQ, UU, and QU covariances (if the EB covariance is diagonal in Fourier space.)

I think this needs to be implemented separately for FlatS2s and FlatS02s?

@cailmdaley cailmdaley self-assigned this Sep 23, 2020
@marius311
Copy link
Owner

Awesome, thanks! Couple of minor comments:

  • Can you use @match and also just do getindex instead of Base.getindex so it basically matches the style of the function right above it? (at some point I'll probably switch the entire codebase to the Base.getindex style which is probably better, but for now I just want to keep it homogenous)

  • Can you add :UQ for completeness?

  • Can you add a test? I'm thinking something along the lines of:

    C :: Diagonal{<:FlatEBFourier} = ...
    f :: FlatS2 = ...
    
    @test C*f  FlatQUFourier(C[:QQ]*f[:Q]+C[:QU]*f[:U], C[:UU]*f[:U]+C[:UQ]*f[:Q])

For the ParamDependentOp, I don't think that'll quite work since you haven't changed the recompute_function, so eg C[:QQ](θ) would actually return a Diagonal{<:FlatEBFourier} still. I think its probably clearest to follow the convention with these ParamDependentOps which is that as soon as you do anything to them, they lose their param-dependence-ness and get evaluated at the fiducial parameters. This is basically what

for F in (:inv, :pinv, :sqrt, :adjoint, :Diagonal, :diag, :simulate, :zero, :one, :logdet, :global_rng_for)
@eval $F(L::ParamDependentOp) = $F(L.op)
end
does. Maybe right below there, add something like getindex(L::ParamDependentOp, x) = getindex(L.op, x), so that C[:QQ] if C::ParamDependentOp will just evaluate it at fiducial parameters and then return the :QQ block.

@marius311
Copy link
Owner

For C::FlatS02, since C[:P] already returns a Diagonal{<:FlatEBFourier} with the P block, I think it can be a one-line

(:QQ || :UU || :QU || :UQ) => getindex(L[:P], k)

added to

:IP => L
:I => L.ΣTE[1,1]
:E => L.ΣTE[2,2]
:B => L.ΣB
:P => Diagonal(FlatEBFourier(L[:E].diag, L[:B].diag))
_ => throw(ArgumentError("Invalid FlatIEBCov index: $k"))

Also, I realize now there's an inconsistency since its C[:E] but its C[:QQ]. I think your convention is clearer though, I will try to make it C[:EE] in a followup (after I think if that breaks anything badly, don't think so...)

@cailmdaley
Copy link
Collaborator Author

Thanks Marius, everything now works out as you suggested. It basically wrote itself!

I'm now also thinking about adding :TQ and :TU indexing, and maybe even :QE, :QB, :UE, :UB (I'm wondering if you could use a GP to delens, filter, and do the QU -> EB transform all in one map-space step..). If this seems reasonable I can take a stab at it.

@marius311 marius311 merged commit 54af99d into master Sep 26, 2020
@marius311
Copy link
Owner

Thanks again! I agree the other stuff would make sense to have too, would be happy to take.

@marius311 marius311 deleted the qu_covariance branch March 20, 2021 05:17
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.

None yet

2 participants