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

pspecbeam.PSpecBeamUV updates for efield beam #143

Merged
merged 6 commits into from
Jul 1, 2018
Merged

Conversation

nkern
Copy link
Member

@nkern nkern commented Jun 29, 2018

Updates the beam files in the hera_pspec/data directory, and allows pspecbeam.PSpecBeamUV to take an efield beam and convert to power beams.

@ghost ghost assigned nkern Jun 29, 2018
@ghost ghost added the in progress label Jun 29, 2018
@nkern nkern requested a review from Chuneeta June 29, 2018 08:58
Copy link
Collaborator

@philbull philbull left a comment

Choose a reason for hiding this comment

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

Changes look good to me. I think @Chuneeta had some comments too though.

@@ -429,7 +440,7 @@ def power_beam_sq_int(self, pol='pI'):
primary_beam_area: float, array-like
"""
if hasattr(self.primary_beam, 'get_beam_area'):
return self.primary_beam.get_beam_sq_area(pol)
return np.real(self.primary_beam.get_beam_sq_area(pol))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this function be returning complex numbers to begin with? Or is there a bug in get_beam_sq_area?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was asking @bhazelton, and I never got a response, but I think I came to the conclusion that in principle it can be complex (e.g. when asking for XY and YX areas), but in practice we will always be using XX or YY or pI or pQ beams, in which case it is real. But @bhazelton let me know if you have any opinions on that.

Copy link
Collaborator

@Chuneeta Chuneeta left a comment

Choose a reason for hiding this comment

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

looks good! Just a comment that needs to be implemented.

# setup primary power beam
self.primary_beam = uvb
if uvb.beam_type == 'efield':
self.primary_beam.efield_to_power(inplace=True)
Copy link
Collaborator

Choose a reason for hiding this comment

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

efield_to_power is called for linear polarizations, for pseudo-stokes you need to use the function efield_to_pstokes.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this is done assuming one only wants linear dipole polarization. This function is (purposely) not trying to be smart and will not convert the efield to pstokes. To pspecbeam for pstokes parameters, one needs to feed a pstokes power beam. (see the unit tests added where I test for exactly this case). I'll make a comment specifying this in the docstring!

Copy link
Collaborator

@philbull philbull left a comment

Choose a reason for hiding this comment

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

Looks good. Let's get this merged.

@philbull philbull dismissed Chuneeta’s stale review July 1, 2018 16:04

Requested changes have now been made.

@philbull philbull merged commit 472be38 into master Jul 1, 2018
@ghost ghost removed the in progress label Jul 1, 2018
@nkern nkern deleted the pstokes_beam_updates branch July 2, 2018 00:46
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

3 participants