Read in art products for wire-cell imaging #36
Conversation
|
A new Pull Request was created by @ebelchio12 (Ewerton Belchior) for develop. It involves the following packages: larwirecell @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
The code-checks are being triggered in jenkins. |
|
-code-checks Then commit the changes and push them to your PR branch. |
|
@ebelchio12 Please apply the clang format as mentioned here: #36 (comment) |
|
Hi @ebelchio12 Notes from our meeting
|
|
Pull request #36 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again. |
|
The code-checks are being triggered in jenkins. |
|
+code-checks |
| cfg["wiener_inputTag"] = ""; // art tag for wiener recob::Wire | ||
| cfg["gauss_inputTag"] = ""; // art tag for gauss recob::Wire |
There was a problem hiding this comment.
I'll just make one big comment but it applies to essentially this whole diff.
There are two main problems with this PR that must be cleaned up. They are like we discussed on zoom and I commented on in the PR.
-
This class must work for a wide variety of users and so should not have any application specific information that is hard-wired in C++ or in configuration. Ie, there should be no configuration keys nor C++ symbols that include the label
wienerorbad, etc. Instead, the C++ should be purely "data driven" by the configuration. -
There should be no "modal"
true/falseconfiguration values. Instead, if some action is to be taken by the class then the configuration values needed to perform that action will be given. If no such configuration values are given then the class simply does not take the action.
Specifically:
-
Remove all the
*_inputTagconfiguration and code that touches them. There is already aframe_tagsconfiguration and that is generic and is interpreted by convertingart::Eventdata into frame traces. -
Remove the current
cmm_tag(andbadmasks_inputTag) configuration and all code that touches them. What the user must supply is a instead mapping that associates a CMM key (eg"bad"to anart::Eventlabel. I would call this configuration parameter, perhaps,cmm_entries. -
Remove
m_cmm_setupand instead convert CMM information (using your existing by iterating on the newcmm_entriesconfiguration. If that configuration is empty then so is the loop body. The actual conversion code you have at the end of this diff (the loop overbad_masks, but again, the symbol "bad" is ... bad) looks okay to me.
| for (auto bad_mask : bad_masks) { | ||
| Waveform::ChannelMasks cm; | ||
| size_t nchannels = bad_mask.second->size() / 3; | ||
| for (size_t i = 0; i < nchannels; i++) { | ||
| size_t ch_idx = 3 * i; | ||
| size_t low_idx = 3 * i + 1; | ||
| size_t up_idx = 3 * i + 2; | ||
| auto cmch = bad_mask.second->at(ch_idx); | ||
| auto cm_first = bad_mask.second->at(low_idx); | ||
| auto cm_second = bad_mask.second->at(up_idx); | ||
| Waveform::BinRange bins(cm_first, cm_second); | ||
| cm[cmch].push_back(bins); | ||
| } | ||
| cm_out = Waveform::merge(cm_out, cm); | ||
| } | ||
|
|
||
| cmm[m_cmm_tag] = cm_out; // for now use a single tag for output cmm |
There was a problem hiding this comment.
I think this is mostly okay but do not use the symbol bad and save into the cmm using the key from the cmm_entries configuration and not any singular name (ie, remove m_cmm_tag entirely).
|
@ebelchio12, is there any update on this? |
No description provided.