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

Enabling output of dof from autochisq.values() #4752

Merged
merged 5 commits into from
Jun 5, 2024

Conversation

pannarale
Copy link
Contributor

@pannarale pannarale commented May 17, 2024

Unlike other chi-squared values methods, autochisq.values() computes the number of degrees of freedom but does not return it. This PR makes the situation even across the values methods of the various chi-squares.

This change affects: PyGRB (autochi-square plots can now be generated) and pycbc_inspiral (coincident searches) but the degrees of freedom are dumped to _ so nothing meaningful is modified.

Testing performed

A full PyGRB workflow with the generation of autochi-square plots was successfully carried out.

  • The author of this pull request confirms they will adhere to the code of conduct

@pannarale pannarale added the PyGRB PyGRB development label May 17, 2024
@pannarale pannarale requested a review from spxiwh May 17, 2024 08:51
@pannarale pannarale self-assigned this May 17, 2024
@pannarale pannarale mentioned this pull request May 21, 2024
1 task
Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

I think it makes sense that autochisq be consistent with other chi-squareds. Not sure how useful the DOF parameter would be though, once the value of the auto-chisq is normalized.

(
ifo_out_vals['auto_chisq'],
ifo_out_vals['auto_chisq_dof'],
) = autochisq.values(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work? I'd expect ifo_out_vals['auto_chisq_dof'] to be a single integer, and ifo_out_vals['auto_chisq'] to be an array. Would this cause issues later if this isn't handled correctly?

... While I'm happy for this function to return the DOF parameter for consistency, I'm not sure it's going to be useful for much (assuming the auto_chisq value is already divided by DOF).

Copy link
Contributor Author

@pannarale pannarale Jun 4, 2024

Choose a reason for hiding this comment

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

Yes, it works, no there are no issues because the numpy.repeat call in this case is here, which replicates this other line.
Let me know if you want me to move the calls from these two lines to pycbc/vetoes/autochisq.py in the spirit of make things even across the chi-squares. I have to push another commit in any event because I noticed that this description is incomplete.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, if there is a numpy.repeat then this is not a concern.

@pannarale pannarale requested a review from spxiwh June 4, 2024 15:50
Copy link
Contributor

@spxiwh spxiwh left a comment

Choose a reason for hiding this comment

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

I'm happy with this now. Thanks!

@pannarale
Copy link
Contributor Author

Thank you!

@pannarale pannarale merged commit aedff9a into gwastro:master Jun 5, 2024
32 of 33 checks passed
@pannarale pannarale deleted the autochisq_dof branch June 6, 2024 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PyGRB PyGRB development
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants