Multiifo ranking statistic #2689
Conversation
|
@GarethDaviesGW in general, you should not be repeating code blocks from existing classes. You can inherit their methods by making the new class a subclass/child class of them, or (less preferable) by explicitly calling a method of an existing class. |
GarethCabournDavies
force-pushed
the
multiifo_ranking_statistic
branch
from
d2bebb2 to
8a6d72d
Compare
…ace for testing. Ignore these and they will be removed
GarethCabournDavies
force-pushed
the
multiifo_ranking_statistic
branch
from
ea757e4 to
06b0e38
Compare
|
Scatter vs exp_fit_sg_csnr: histograms of the two statistics: |
GarethCabournDavies
force-pushed
the
multiifo_ranking_statistic
branch
from
bb59594 to
7a715f4
Compare
GarethCabournDavies
force-pushed
the
multiifo_ranking_statistic
branch
from
7a715f4 to
87d5527
Compare
pycbc/events/coinc_rate.py
Outdated
|
|
||
| # if more than one possible coincidence type exists, | ||
| # calculate coincidence for subsets through recursion | ||
| if len(ifos) > 2: | ||
| ifos = numpy.array(sorted(rates.keys())) |
There was a problem hiding this comment.
any reason why they need to be sorted?
There was a problem hiding this comment.
This line is not actually needed, as it is just repeating line 39
In that case, it is needed to ensure consistency for the ordering of the ifostring dict key. Otherwise I think the ordering is not stable
The conversion to a numpy array however is not needed and is removed
pycbc/events/coinc_rate.py
Outdated
| Value is expected coincidence rate in the combination, units Hz | ||
| """ | ||
| # extract ifo list and rates vectors from the dictionary | ||
| ifos = numpy.array(rates.keys()) |
There was a problem hiding this comment.
does ifos have to be a numpy array? the noise coincident area function says it takes a list of ifos.
similarly, does it work to just use rates.values() for the rate vectors?
There was a problem hiding this comment.
You're right, ifos doesn't have to be a numpy array
Aslo, rates.values() works and I have updated
Testing this on my laptop rather than the cluster (by accident) has shown that the behaviour of dictionary.keys() and dictionary.values() will change in python 3; in python 3 they give specific dict_keys and dict_values objects (tagging @spxiwh in this as he has been working on python 3 updates)
As such, for future-proofing, I have included a list() wrapper around the .keys() call, which does nothing in python 2 but changes them back to being a list in python 3
The dictionary.values() being a dict_values object rather than list doesn't matter in this case as it is being looped over and acts in exactly the same way
I have done the same on line 39 to convert the keys to a list
There was a problem hiding this comment.
OK. It looks like list(mydict) would be the same as list(mydict.keys()) so that's one less pair of brackets. I'll edit this in the browser and then I think we're finished for this iteration
|
will merge assuming that 'final' tests work out. |
* initial commit of coinc_rate stat * update to an actual working statistic * codeclimate changes, change to use SG veto in coinc_rate statistic * some reworking of statistic, a bit of housekeeping with coinc_rate calculation * some changes for clening up code - some print statements may be in place for testing. Ignore these and they will be removed * some tidying up * commit for changing branch * use count above thresh as that is how fitting calculates * accidentally copied some slack chat into the code * codeclimate again * some changes to master not integrated to branch * updates to comments, some casting to arrays not needed. Small future-proofing for python 3 * be more numpy-y, add minor comment
Multiifo ranking statistic:
Currently calculates the expected rate of coincidences at a given snr value, and subtracts this from a mean coincidence rate (this is a rate for all coincidences, not including the snr infromation) in that detector combination