Some changes to HFile.select() and SingleDetTriggers #4545
Conversation
|
Testing on the updates to the executables which use these are ongoing. I will point to the success (or make changes) when they are complete |
|
I am going to try and un-break the filter string option as the next step in this |
GarethCabournDavies
force-pushed
the
hfile_select_ranking
branch
2 times, most recently
from
4b9327e to
b3b6073
Compare
|
Okay, I think this is ready to review now |
GarethCabournDavies
changed the title
| @@ -299,6 +299,21 @@ def get_newsnr_sgveto_psdvar_scaled_threshold(trigs): | |||
| 'newsnr_sgveto_psdvar_scaled_threshold': get_newsnr_sgveto_psdvar_scaled_threshold, | |||
| } | |||
|
|
|||
| # Lists of datasets required in the trigs object for each function | |||
| required_datasets = {} | |||
There was a problem hiding this comment.
Does it make this linked to the function themselves? I'm just wondering if this is really maintainable.
There was a problem hiding this comment.
These are not linked, the dictionary will need updating as more functions are added into this module.
One way to add this would be if we define the dictionary before the functions, and all functions have an assert that the required_datasets are in trigs. Then if another function is made (likely copied and adapted), then people will naturally add the list into this dictionary. This doesn't make it more maintainable, but does add a psychological incentive to add the list
There was a problem hiding this comment.
I've added the check, so anyone using get_sngls_ranking_from_trigs without adding the required datasets will have a problem
There was a problem hiding this comment.
I've had to abandon this idea, as the get_sngls_ranking_from_trigs function is used by quite a few different input classes for trigs, so making the check general enough was over-complicated (in my mind at least)
|
@GarethCabournDavies I don't see too much to comment on anymore,, except the one thing. Also can you comment or resolve the issues above? It's not quite clear what is still be working on and what the choice made was in response. |
I was leaving them open until I have finished testing and committed, but will resolve as soon as possible |
|
Noticed an error in the way pre-masks are handled alongside the new filter_rank in SingleDetTriggers (it's not actually used anywhere in the codebase, but just to be safe), I think I have the fix ready, but this is not to merge before then |
|
Fix to the above is in, so please approve if you are happy |
|
Where are we with this, looks like just waiting for review? |
Yup - the last update was in the day or so before christmas break, so not many working days to have had a look at this |
|
Poke for review on this |
|
... And there's now conflicts. |
…equired dataset list if they add a new ranking\!)
…ifferent types use the same function
GarethCabournDavies
force-pushed
the
hfile_select_ranking
branch
from
5645f39 to
7d0809d
Compare
|
@ahnitz Poke |
* Some changes to HFile.select() and mask_to_n_loudest_clustered * missed comma * Some CC problemas * Update * revert changes which meant kwargs couldnt be passed to mask_to_n_loudest_clustered_events * CC / fixes * Use list rather than numpy array for mask * remove messing about with the mask dtyping * initialise a transparent mask if it is not given * Fix problem when the filter value may not be available to the HFile * thinko in pycbc_bin_trigger_rates_dq * Don't make a huge boolean mask of all True values and then run nonzero on it = BAD IDEA * CC * re-enable option to have no mask * efficiency saving in pycbc_plot_hist * no mask option in trig_dict * typo * allow None maks in mask_size * thought i could apply another mask but I cant * get filter_func back, use new interface for recent changes * Some minor fixes * cc * allow option to do a logical and on masks * various minor bugfixes * max z problem * fixes to pycbc_page_snglinfo * use alternative dict for beter clarity * IWH comments * add check for required datasets (mostly to remind people to add the required dataset list if they add a new ranking\!) * fix bad check, remove min_snr option * remove option from CI * Fix bug with applying premask and using filter_rank threshold at the same time * stop checking for required datasets - this is difficult when so may different types use the same function * CC prefers np.flatnonzero(arr) to arr.nonzero()[0] * Fix a couple of rarely-used scripts, remove unused imports, whitespace
* Some changes to HFile.select() and mask_to_n_loudest_clustered * missed comma * Some CC problemas * Update * revert changes which meant kwargs couldnt be passed to mask_to_n_loudest_clustered_events * CC / fixes * Use list rather than numpy array for mask * remove messing about with the mask dtyping * initialise a transparent mask if it is not given * Fix problem when the filter value may not be available to the HFile * thinko in pycbc_bin_trigger_rates_dq * Don't make a huge boolean mask of all True values and then run nonzero on it = BAD IDEA * CC * re-enable option to have no mask * efficiency saving in pycbc_plot_hist * no mask option in trig_dict * typo * allow None maks in mask_size * thought i could apply another mask but I cant * get filter_func back, use new interface for recent changes * Some minor fixes * cc * allow option to do a logical and on masks * various minor bugfixes * max z problem * fixes to pycbc_page_snglinfo * use alternative dict for beter clarity * IWH comments * add check for required datasets (mostly to remind people to add the required dataset list if they add a new ranking\!) * fix bad check, remove min_snr option * remove option from CI * Fix bug with applying premask and using filter_rank threshold at the same time * stop checking for required datasets - this is difficult when so may different types use the same function * CC prefers np.flatnonzero(arr) to arr.nonzero()[0] * Fix a couple of rarely-used scripts, remove unused imports, whitespace
* Some changes to HFile.select() and mask_to_n_loudest_clustered * missed comma * Some CC problemas * Update * revert changes which meant kwargs couldnt be passed to mask_to_n_loudest_clustered_events * CC / fixes * Use list rather than numpy array for mask * remove messing about with the mask dtyping * initialise a transparent mask if it is not given * Fix problem when the filter value may not be available to the HFile * thinko in pycbc_bin_trigger_rates_dq * Don't make a huge boolean mask of all True values and then run nonzero on it = BAD IDEA * CC * re-enable option to have no mask * efficiency saving in pycbc_plot_hist * no mask option in trig_dict * typo * allow None maks in mask_size * thought i could apply another mask but I cant * get filter_func back, use new interface for recent changes * Some minor fixes * cc * allow option to do a logical and on masks * various minor bugfixes * max z problem * fixes to pycbc_page_snglinfo * use alternative dict for beter clarity * IWH comments * add check for required datasets (mostly to remind people to add the required dataset list if they add a new ranking\!) * fix bad check, remove min_snr option * remove option from CI * Fix bug with applying premask and using filter_rank threshold at the same time * stop checking for required datasets - this is difficult when so may different types use the same function * CC prefers np.flatnonzero(arr) to arr.nonzero()[0] * Fix a couple of rarely-used scripts, remove unused imports, whitespace
* Some changes to HFile.select() and mask_to_n_loudest_clustered * missed comma * Some CC problemas * Update * revert changes which meant kwargs couldnt be passed to mask_to_n_loudest_clustered_events * CC / fixes * Use list rather than numpy array for mask * remove messing about with the mask dtyping * initialise a transparent mask if it is not given * Fix problem when the filter value may not be available to the HFile * thinko in pycbc_bin_trigger_rates_dq * Don't make a huge boolean mask of all True values and then run nonzero on it = BAD IDEA * CC * re-enable option to have no mask * efficiency saving in pycbc_plot_hist * no mask option in trig_dict * typo * allow None maks in mask_size * thought i could apply another mask but I cant * get filter_func back, use new interface for recent changes * Some minor fixes * cc * allow option to do a logical and on masks * various minor bugfixes * max z problem * fixes to pycbc_page_snglinfo * use alternative dict for beter clarity * IWH comments * add check for required datasets (mostly to remind people to add the required dataset list if they add a new ranking\!) * fix bad check, remove min_snr option * remove option from CI * Fix bug with applying premask and using filter_rank threshold at the same time * stop checking for required datasets - this is difficult when so may different types use the same function * CC prefers np.flatnonzero(arr) to arr.nonzero()[0] * Fix a couple of rarely-used scripts, remove unused imports, whitespace
This is an attempt to sort #4524
These changes allow:
HFile.select()functionHFile.select()functionality can used for simple things easilymask_to_n_loudest_clustered_eventsin cases where this is appropriateSo far as I can test, this has not broken previous functionality except for one usage, which is the filter string option in
pycbc_plot_singles_vs_params