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

Purge fullinj / injfull analyses #4420

Merged
merged 5 commits into from
Jul 26, 2023

Conversation

GarethCabournDavies
Copy link
Contributor

Injections do not have inclusive IFAR / FAP calculated, so we can ignore the datasets.

Why?:

  • The injfull/fullinj coinc jobs uses a lot of computation and these are thrown away
  • the coinc_statmap_inj jobs are a bottleneck which can spend a lot of time on fullinj/injfull without any value

Proposed solution:

  • Don't run injfull/fullinj analyses (done through the workflow generator)
  • output empty arays from the statmap jobs for inclusive IFAR/FAP to indicate that this wasn't calculated on purpose

Note that I think we may be able to consolidate the statmap / statmap_inj jobs in the near future because of this change, but this isn't done here.

examples/search/plotting.ini Outdated Show resolved Hide resolved

# Indicate that inclusive IFAR is not calculated
f['foreground/ifar'] = numpy.array([])
f['foreground/fap'] = numpy.array([])
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be in favour of not adding empty arrays into the coinc files and just removing these lines (and in sngls_statmap). Unless it serves a specific purpose??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for this is intended to be that this can be used to indicate that the inclusive IFAR is specifically not calculated. If there is no dataset, that could indicate an error/bug, but a specifically empty dataset implies that it was done on purpose (we could alternatively use all zero/NaN/inf values)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I see it the other way. An empty column to me indicates an error/bug more than the column not existing. And purging something means that it never existed, and it never will exist, so I should never look for it.

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.

This looks good to me. @ahnitz Any objections here?

@spxiwh spxiwh merged commit 0c0819d into gwastro:master Jul 26, 2023
16 checks passed
@GarethCabournDavies GarethCabournDavies deleted the purge_inj_inclusive branch August 3, 2023 15:25
PRAVEEN-mnl pushed a commit to PRAVEEN-mnl/pycbc that referenced this pull request Nov 3, 2023
* Purge fullinj / injfull analyses

* remove unrelated change

* Actually purge

* delete del deletion

* foreground_time may not be [present
bhooshan-gadre pushed a commit to bhooshan-gadre/pycbc that referenced this pull request Mar 4, 2024
* Purge fullinj / injfull analyses

* remove unrelated change

* Actually purge

* delete del deletion

* foreground_time may not be [present
acorreia61201 pushed a commit to acorreia61201/pycbc that referenced this pull request Apr 4, 2024
* Purge fullinj / injfull analyses

* remove unrelated change

* Actually purge

* delete del deletion

* foreground_time may not be [present
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants