Fix sngls_minifollowup event selection/ordering [master] #4823
Conversation
|
@yi-fan-wang You may want to incorporate this fix. I think it is probably the thing we were just looking at! |
|
@GarethCabournDavies Hi! Are the events in the mini follow-up page still the loudest ones but just randomly ordered, or are they not even the loudest events? |
|
The ones before this fix are just random ones, not even the loudest |
GarethCabournDavies
force-pushed
the
master_singles_minifollow_order
branch
from
5cf0e61 to
24edb62
Compare
There was a problem hiding this comment.
The changes look good to me. However when reviewing this file, I found issues not related to this PR, (1) events are not defined (see this line) (2) a minor issue that coinc is imported but not used anywhere.
There was a problem hiding this comment.
Thanks for catching that! Those are the types of things which would be caught by #4638, so hopefully that won't happen too much in future
I will fix those bugs and then merge
* Fix sngls_minifollowup event selection/ordering [master] * Remove testing printing * Still create stat class variable even when there are no triggers * Ranking -> statistic rename * minor bugs caught duiring review * oops, accidentally added to this PR
We noticed that the sngls_minifollowup pages had essentially randomly chosen, and randomly-ordered events.
This was due to a bug introduced by an overzealous approach to the scope creep part of #4549 (my fault!)
This will also be fixed on the v23_release_branch ... coming soon
Standard information about the request
This is a bug fix
This change affects the offline search
This change changes result presentation
This change follows style guidelines (See e.g. PEP8), has been proposed using the contribution guidelines
Contents
Return to using the mask_to_n_loudest_clustered_events function rather than trying anything too fancy
Make it so that mask_to_n_loudest_clustered_events can use a ranking statistic rather than sngl-ranking
Testing performed
Currently running given latest version
Run pycbc_sngl_minifollow with ranking-statistic both as single-ranking-only (default) and as something more complicated
Check by printing values that the top events are in the right order, and that the are the highest-statistic events
The author of this pull request confirms they will adhere to the code of conduct