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

Refactoring exact norm #217

Merged
merged 9 commits into from
Aug 9, 2019
Merged

Refactoring exact norm #217

merged 9 commits into from
Aug 9, 2019

Conversation

RonanLegin
Copy link
Contributor

In pspecdata.py, in the definition of q_hat (line 956), there is a for loop present (line 1052) in which within this loop, we calculate Q as Q = del_tau * self.get_Q(i, pol). This for loop is going over each delay mode "i" in which get_Q takes as a parameter. If we go to the function get_Q (around line 1557), it computes the exponential term and the integral of the primary beam. However, the integral does not depend on the delay mode. So back in the q_hat function, the integral is being calculated in the loop for every delay mode when in fact its result is constant. To change this, I created a separate function in pspecdata.py called get_integral_beam where the code that was in get_Q to compute the integral has been transferred to this new function. Outside of the for loop, I call get_integral_beam and store its result in a variable called integral_beam and then multiply it inside the loop as Q = del_tau * self.get_Q(i, pol) * integral_beam. This way we are always using the same result for each delay mode. This makes generating the power spectrum faster using exact_norm = True as a parameter in pspec. This change does not affect the time to generate the power spectrum when exact_norm is set to "False", as it doesn't use this for loop in q_hat. I also wrote a test function called test_get_integral_beam in test_pspecdata.py for my new function get_integral_beam.

@coveralls
Copy link

coveralls commented Aug 1, 2019

Coverage Status

Coverage increased (+0.002%) to 96.819% when pulling 69c0d78 on refactoring_exact_norm into 7708f92 on master.

@saurabh-astro saurabh-astro self-assigned this Aug 6, 2019
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/pspecdata.py Outdated Show resolved Hide resolved
hera_pspec/tests/test_pspecdata.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@saurabh-astro saurabh-astro left a comment

Choose a reason for hiding this comment

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

Looks great. I have made minor suggestions related to 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.

I agree with @saurabh-astro's comments -- this is ready to go once those docstrings and comments have been updated.

@philbull
Copy link
Collaborator

philbull commented Aug 7, 2019

There was a merge conflict, so I fixed that and did a bit of tidy-up. @Ronan1879 don't forget to pull before making any more changes!

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!

@philbull philbull merged commit 2c248d1 into master Aug 9, 2019
@philbull philbull deleted the refactoring_exact_norm branch August 9, 2019 10:25
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.

4 participants