Feature/aantonak crtveto

[aantonakis]

Added a simple SBND CRT Veto Class to store boolean values for each art event. The different boolean values correspond to different sets of CRT Veto logic based on CRT hit information. The CRT veto logic is implemented as a producer module in sbndcode.

[henrylay97]

Sorry apparently I never actually submitted my review last week - the comments were listed as pending.

One of them you've already addressed which is great :D

[henrylay97]

Would it be best to store this a vector of bools, with the indexing being responsible for the numbering scheme.

With carefully designed constructors & methods this would allow you to make changes to the veto categories (thinking mainly additions) in future without having to edit anything about the object.

Even better we could make it a map and the key being a enumerated integer such that edits wouldn't risk changing the meaning of each key. @kjplows any thoughts?

[henrylay97]

@aantonakis @kjplows thoughts on this one?

[kjplows]

Hi folks, apologies on the late reply - I like the idea @henrylay97 suggests of using an enum; especially since the names V0 .. 4 aren't particularly meaningful without the context of DocDB 40318, 40602.

On the idea of a map: @henrylay97, if we were happy to standardise an enum now (say for argument's sake typedef enum t_CRTVetoType { kExitingCCOneTop = 0, ... } CRTVetoType_t; we could add arbitrarily many members, but we would have to be careful about setting a consistent naming scheme that's descriptive here. Regardless of if we had a map <enum, bool> or a raw enum, we'd still have to maintain a dictionary somewhere..

I wonder if, instead, this class could map to a std::bitset like the table @aantonakis has on Slide 24 of 40318; something like abcd where a == "Extended" or not, b == "North" or not, c == "Both tops" or not, d == "South" or not. Then one could assign something like

typedef enum CRTVetoType
{
    kExitingCCOneTop = 0,
    kExitingCCBothTops = 1,
    kNoExitingCCOneTop = 2,
    kNoExitingCCBothTops = 3,
    kDirtTag = 4
} CRTVetoType_t;

and a map to go along with it (I use a hashmap here because it's fast but map is fine too);

std::unordered_map<CRTVetoType_t, std::bitset<4>> _vetoMap =
{
  std::pair<CRTVetoType_t, std::bitset<4>>( kExitingCCOneTop, std::bitset<4>{"1100"} ),
  std::pair<CRTVetoType_t, std::bitset<4>>( kExitingCCBothTops, std::bitset<4>{"1110"} ),
  std::pair<CRTVetoType_t, std::bitset<4>>( kNoExitingCCOneTop, std::bitset<4>{"1000"} ),
  std::pair<CRTVetoType_t, std::bitset<4>>( kNoExitingCCBothTops, std::bitset<4>{"1010"} ),
  std::pair<CRTVetoType_t, std::bitset<4>>( kDirtTag, std::bitset<4>{"0001"} )
};

This would then reduce the requirement that you explicitly maintain a dictionary with each tag having a particular meaning, and instead just puts in a table like Alex has. Plus if you're adding stuff and you add in more walls, you just increase the size of the bitset (or maybe you just increase it now and make the table larger with many possibilities currently unused), and you don't necessarily need to come up with "descriptive" names for an enum. Plus you don't break keys!
(Thoughts? If you think you'll have multiple time windows, @aantonakis , then maybe this is suboptimal)

[kjplows]

Oh and while we're at it, a comment I saw Gianluca make some time ago: you could consider upgrading an enum to enum class CRTVetoType instead, so all references to that enum would be CRTVetoType vv = CRTVetoType::kExitingCCOneTop if you wanted to ensure these vetoes (vetos?) didn't clash with anything else that might be named in the codebase.

[henrylay97]

On the final version of this PR we can keep just the latest checksum and call it version 10. We only really need to keep the backward compatibility for objects that have been in the workflow already :D

[henrylay97]

@aantonakis @kjplows thoughts on this one?

[henrylay97]

Useful debugging tool but are you keeping this? Could make the object a lot heavier? And is duplicate information really given the association is being made to the spacepoints.

[kjplows]

Great, thanks Alex! Good to have the documentation in 💪

[kjplows]

Thanks!

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for e26:prof -- details available through the CI dashboard

[FNALbuild]

✔️ CI build for LArSoft Succeeded on slf7 for c14:prof -- details available through the CI dashboard

[FNALbuild]

❌ CI build for SBND Failed at phase build SBND on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build SBND phase logs

parent CI build details are available through the CI dashboard

[FNALbuild]

❌ CI build for ICARUS Failed at phase build ICARUS on slf7 for c14:prof -- details available through the CI dashboard

🚨 For more details about the failed phase, check the build ICARUS phase logs

parent CI build details are available through the CI dashboard

[FNALbuild]

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests SBND phase logs

parent CI build details are available through the CI dashboard

[FNALbuild]

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored warnings for build -- details available through the CI dashboard

🚨 For more details about the warning phase, check the ci_tests ICARUS phase logs

parent CI build details are available through the CI dashboard