Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

added Left and Right Circular Projection matrix #3599

Merged
merged 19 commits into from
Feb 2, 2022

Conversation

JAMSADIQ
Copy link
Contributor

Added a new function in this code for left and right circular polarization projection matrix.

@spxiwh
Copy link
Contributor

spxiwh commented Jan 27, 2021

@a-r-williamson I've assigned this one to you to look over.

@JAMSADIQ
Copy link
Contributor Author

With the above commits now pycbc-multi-inspiral and lalapps_coh_PTF_inspiral code give same results when using left anf right circular polarization matrices. The details can be found at https://wiki.ligo.org/Bursts/CircularlyPolarizedProjectionMatrix
In order to check the code for accepting merge request my modified code can be used for an injection test with command line options given in https://wiki.ligo.org/Bursts/CircularlyPolarizedProjectionMatrix page.

@@ -107,6 +107,7 @@ parser.add_argument("--null-step", type=float, default=20., help="""
parser.add_argument("--trigger-time", type=int, help="""
Time of the GRB, used to set the antenna patterns.""")

parser.add_argument("--projection-type", default='standard', type=str, help="""Choice of projection matrix with standard, left circular or right circular""")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
parser.add_argument("--projection-type", default='standard', type=str, help="""Choice of projection matrix with standard, left circular or right circular""")
parser.add_argument("--projection-type", default="standard",
choices=["standard", "left", "right", "left+right"],
help="Choice of projection matrix. 'Left' and 'right' "
"correspond to face-away and face-on.")

@@ -211,6 +211,16 @@ def get_projection_matrix(wp, wc):
return projection_matrix


def circular_projection_matrix(wp, wc):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we perhaps unify this into the above function get_projection_matrix which could have a kwarg for the projection name?

@@ -513,6 +528,10 @@ with ctx:
sigma = {ifo : np.sqrt(sigmasq[ifo]) for ifo in opt.instruments}
# Every time s_num is zero or we skip the segment, we run new
# template to increment the template index
####### Testing values
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
####### Testing values

####### Testing values
wp,wc=get_weighted_antenna_patterns(Fp,Fc,sigma)
projection_matrix = get_projection_matrix(wp,wc)
LCprojection_matrix, RCprojection_matrix = circular_projection_matrix(wp, wc)
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to account for the normal GRB search behaviour where both right and left projections are used.

@JAMSADIQ
Copy link
Contributor Author

I make changes and push them. Kindly check it

return left_projection_matrix, right_projection_matrix

else:
logging.info("choice of projection matrix is not available")
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be an exception, not a logging.info() call:

raise ValueError('unknown projection type ' + str(projection_type))


if projection_type=='standard':
logging.info("choice of projection matrix is %s"
% str(projection_type))
Copy link
Contributor

@titodalcanton titodalcanton Nov 16, 2021

Choose a reason for hiding this comment

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

Calls to logging.info() should not use the string formatting operations (% and .format()). Instead, do this:

logging.info("choice of projection matrix is %s", projection_type)

(this comment also applies to logging.info() calls further below).

@JAMSADIQ
Copy link
Contributor Author

Fixed some issues for review.

Copy link
Contributor

@a-r-williamson a-r-williamson left a comment

Choose a reason for hiding this comment

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

Hi @JAMSADIQ, thanks for the recent commits. I've requested a few minor changes here, which hopefully you can just click accept on then I can approve the PR.

Comment on lines 656 to 657
logging.info("%s points above coherent threshold"
% str(len(rho_coh)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logging.info("%s points above coherent threshold"
% str(len(rho_coh)))
logging.info("%d points above coherent threshold", len(rho_coh))

Comment on lines 614 to 615
logging.info("Max coincident SNR = %s"
% str(max(rho_coinc)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logging.info("Max coincident SNR = %s"
% str(max(rho_coinc)))
logging.info("Max coincident SNR = %.2f", max(rho_coinc))

JAMSADIQ and others added 2 commits February 2, 2022 17:49
Co-authored-by: Andrew R. Williamson <a-r-williamson@users.noreply.github.com>
@a-r-williamson a-r-williamson enabled auto-merge (squash) February 2, 2022 17:23
@a-r-williamson a-r-williamson merged commit 65c123b into gwastro:master Feb 2, 2022
a-r-williamson added a commit to a-r-williamson/pycbc that referenced this pull request Mar 17, 2022
* added Left and Right Circular Projection matrix

* make some changes in projection matrix

* Deleted the file from the git repository

* removed extra commented lines

* fix bug in new function denominator term

* "added option to save network chi squared in he output hdf file"

* replace reweighted-snr with coherent-snr in clustering; added a new argument in null-snr function to avoid null cuts; and removed some extra comments.

* Update pycbc_multi_inspiral

fix a bracket in pycbc_reweight_snr function

* fixed the argparser option for projection type

* modify projection matrix function adding left and right projection with a kwargs choice. Modify code before coherent snr calculation

* added the option of left_right in projection matrix function and modify code

* fixed some typos

* minor changes

* fixed logging info string formatting operations and use of ValueError rather a message for wrong option inputs

* removed extra function of circular polarization

* make changes in coding several line for recomended coding

* Update bin/pycbc_multi_inspiral

Co-authored-by: Andrew R. Williamson <a-r-williamson@users.noreply.github.com>

* make some suggested minor chnages in a comment of function and few  logging info statments

Co-authored-by: Jam Sadiq <jam.sadiq@ldas-pcdev1.ligo.caltech.edu>
Co-authored-by: Jam Sadiq <jam.sadiq@ldas-pcdev6.ligo.caltech.edu>
Co-authored-by: Andrew R. Williamson <a-r-williamson@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
PyGRB PyGRB development
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants