Update offline source probability for change to mchirp area #4882
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
It still didn't work from Thomas' change, we needed a further fix for the missing gap entry. Also, even if we run the source probabilities offline with the 'include mass gap' option, the output file doesn't have a mass gap entry.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
The first change was necessary but the code still didn't work, it produced the following error:
Traceback (most recent call last):
File "/home/ana.lorenzo/git_repo/offline-analysis/production/o4/helper_scripts/psource_pycbc_offline.py", line 92, in <module>
probs.pop("Mass Gap")
KeyError: 'Mass Gap'
so we needed one more fix for the missing gap entry.
Moreover, even if you run the 'run_probabilities_offline.sh' script with the '--include-mass-gap' option, the output file does not have a 'Mass Gap' entry.
|
OK, given the changes in the interface to mchirp_area.py to make no mass gap the default (45c9560) it seems we would need more work on this script to be able to actually produce classifications with mass gap. However it doesn't seem that there is any need to support this option for offline results, so I will just remove it completely to simplify the script |
Offline probabilities no longer to support mass gap
|
Hopefully it works without any issue after removing all mass gap option related code |
AnaLorenzoMedina
approved these changes
Sep 18, 2024
spxiwh
added
the
v23_release_branch
spxiwh
pushed a commit
to spxiwh/pycbc
that referenced
this pull request
Sep 25, 2024
…4882) * Update offline source probability for changes to mchirp area * Update pycbc_source_probability_offline It still didn't work from Thomas' change, we needed a further fix for the missing gap entry. Also, even if we run the source probabilities offline with the 'include mass gap' option, the output file doesn't have a mass gap entry. * Remove mass gap option entirely Offline probabilities no longer to support mass gap * remove more mass gap code --------- Co-authored-by: AnaLorenzoMedina <114947731+AnaLorenzoMedina@users.noreply.github.com>
spxiwh
added a commit
that referenced
this pull request
Sep 25, 2024
* Update offline source probability for change to mchirp area (#4882) * Update offline source probability for changes to mchirp area * Update pycbc_source_probability_offline It still didn't work from Thomas' change, we needed a further fix for the missing gap entry. Also, even if we run the source probabilities offline with the 'include mass gap' option, the output file doesn't have a mass gap entry. * Remove mass gap option entirely Offline probabilities no longer to support mass gap * remove more mass gap code --------- Co-authored-by: AnaLorenzoMedina <114947731+AnaLorenzoMedina@users.noreply.github.com> * Bump version * Update GitHub actions to use modern versions (#4867) * Depndabot * Try to solve new issue due to upload-artifact@v2 deprecation * Mimic what has been changed for PyPMC * Still failing, try this… * Include a fix for Node16 deprecation as well * Fix one more error * Also update setup-python to v5 * Remove some debugging stuff --------- Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr> * Pin to older ligo.skymap for tests --------- Co-authored-by: Thomas Dent <thomas.dent@usc.es> Co-authored-by: AnaLorenzoMedina <114947731+AnaLorenzoMedina@users.noreply.github.com> Co-authored-by: Tito Dal Canton <tito.dalcanton@ijclab.in2p3.fr>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix a from_cli bug and remove mass gap related options and code, as mass gap classification does not need to be supported for offline calculation
Standard information about the request
This is a: bug fix
This change affects: the offline search
This change changes: ability to produce a scientific result vs no result
This change: follows style guidelines (See e.g. PEP8)
Bug fix
Motivation
Offline source probability code fails to run as it was not updated for a module change
Contents
Try to make it run
Testing performed
To be tested by local install and attempting to run offline source probability
The author of this pull request confirms they will adhere to the code of conduct