Fix storage format of coinc segments in coinc code #2517
Conversation
| logging.info('Incorrect segment format.') | ||
|
|
||
| key = fi['segments'].keys()[0] | ||
| f['%s/segments/end' % key] = fi['segments/%s/end' % key][:] |
There was a problem hiding this comment.
This is a point where consistency would be better - we change from segments/key to key/segments
This change was made because of wanting the attributes associated with the groups for each detector combination. These attributes are not necessarily associated with the segments and as a result we went for the combination/segments ordering
I think that having it as combinations/segments instead throughout would make more sense, (but I wouldn't be too upset if others disagree)
There was a problem hiding this comment.
Please change to use combinations/segments
There was a problem hiding this comment.
Changed to segments/combinations everywhere
| for t in trigs: | ||
| f['segments/%s/start' % trigs[t].ifo], f['segments/%s/end' % trigs[t].ifo] = trigs[t].valid | ||
| # Store coinc segments keyed by detector combination | ||
| f['segments/%s/start' % ''.join(sorted(detectors))], f['segments/%s/end' % ''.join(sorted(detectors))] = trigs[args.pivot_ifo].valid |
There was a problem hiding this comment.
Does detectors need to be sorted? It will come in the order of which trigger file comes first in the command line, is this not already the order we want?
even so - join(sorted(detectors)) could be its own variable as this line is very long
There was a problem hiding this comment.
Having spoken with Tom, this should be kept as sorted alphabetically, in case the pivot/fixed and precedence scheme changes
There was a problem hiding this comment.
btw this line is too long, please split (PEP8!)
tapaimarton
force-pushed
the
segment_handling
branch
from
7148605 to
2a670f4
Compare
tapaimarton
force-pushed
the
segment_handling
branch
from
5d89f24 to
2e3d335
Compare
| @@ -206,14 +206,14 @@ for trigger_file, injection_file in zip(args.trigger_files, | |||
| sim_table.get_column(args.redshift_column)) | |||
|
|
|||
| if multi_ifo_style: | |||
| for key in f.keys(): | |||
| for key in f['segments'].keys(): | |||
| if key == 'foreground': | |||
| continue | |||
There was a problem hiding this comment.
Do we need to check if key is foreground now? we are going into the segments key already
There was a problem hiding this comment.
yes we do, see line 161-165 of pycbc_multiifo_coinc_statmap
bin/hdfcoinc/pycbc_coinc_hdfinjfind
Outdated
| fo[key].attrs['pivot'] = f[key].attrs['pivot'] | ||
| fo[key].attrs['fixed'] = f[key].attrs['fixed'] | ||
| fo[key].attrs['foreground_time_exc'] = f[key].attrs['foreground_time_exc'] | ||
| fo[key].attrs['pivot'] = f['segments/%s' % key].attrs['pivot'] |
There was a problem hiding this comment.
does this make a group called key which just has the attributes hung to it and nothing else? I think this is okay but wanted to be sure
There was a problem hiding this comment.
If the key does not exists, it is created.
There was a problem hiding this comment.
yes to me it seems only the attributes are there specific to the detector combination
| logging.info('Incorrect segment format.') | ||
|
|
||
| key = fi['segments'].keys()[0] | ||
| f['segments/%s/end' % key] = fi['segments/%s/end' % key][:] |
There was a problem hiding this comment.
It would be good to have a check that none of the detector combinations have been used before (if key in f['segments']) and then give an error if they have
I should have included it before, but hadn't thought of it
There was a problem hiding this comment.
That can only occur if the same file is in the input twice, that should never happen, at least within a workflow. The input is 1 file per detector combination. I do not think this is needed.
| for attr_name in fi.attrs: | ||
| f[ifosstring].attrs[attr_name] = fi.attrs[attr_name] | ||
| f['segments/%s' % key].attrs[attr_name] = fi.attrs[attr_name] | ||
|
|
There was a problem hiding this comment.
As we are creating a coinc group for attributes above, then we should do the same here have a key group in this file as well (i.e. f[key].attrs[attr_name])
There was a problem hiding this comment.
We are not creating it, it already exists, the segments are saved in there for the specific combination, hence there is no point in creating a new group.
There was a problem hiding this comment.
Sorry I wasn't clear, I was suggesting that we do create a f[key] group, even though f['segments/%s' % key] exists, for the attributes which are not related to the segments
(as in #2517 (diff))
There was a problem hiding this comment.
ah there is an error in here, as in there can be a segments/foreground from coinc_statmap. This needs to change for the same check as it is later in hdfinjfind. Though the key is the detector combination, so I am not sure what attribute you want to save not specific to detector combination, key-ed by detector combination
There was a problem hiding this comment.
It should be correct now.
There was a problem hiding this comment.
so the pivot and fixed ifos are not segment related, but are combination related, so my suggestion is to have a different group, just called f[key] and not have the 'segments/'
There was a problem hiding this comment.
changed as requested
Correct coping of segments and attributes
Changes segment handling to only store the segments once, keyed by the detector combination.
Since the StatmapData and MultiifoStatmap data has self.seg = f['segments'] I left it like this, but kept the order change in pycbc_multiifo_combine_statmap.