Feature/howard spillquality #390

brucehoward-physics · 2023-10-16T17:05:20Z

Putting together PR for a couple of spill info updates to try to get things into the CAFs and in a way that we can also start to do beam quality cuts. Some questions notes:

See some info in doc-33224 slides 5-13

IMPORTANT (!!) -- I'm not sure if I should be making this PR against develop or v09_72_00_05 e.g. There seem to be a lot of commits present in the latter but not in develop as my changes are only the last few commits...
Think some additional similar changes may be needed in BNB module(s) but probably would be a PR from their side — including Joseph and Jacob L in the conversation here.
What else do we need for validation? (See some info at doc-33224)
Do we want these parts (info.spill_time_s+(info.spill_time_ns/1.0e9)) to instead do something with more precision?
We probably want to talk to NOvA to remember how they handle the POT sums within CAFAna and with spill quality cuts, but this PR is more about getting the info available for use rather than the internal workings. But it's possible that the way of saving them would preclude certain usage patterns.

Tagging some reviewers that I think make sense given this touches trigger info, spill accounting, CAFs, etc.

…he spill quality for the spill that goes with a given event, and code supporting that.

…wants to use this spill. The other checks not just for the first event but the first event OF A GIVEN STREAM for the numi case.

brucehoward-physics · 2023-10-16T17:07:20Z

One other thing to check is that we probably want some way to directly check in the CAFs nspill from ifbeam vs nspill from TDAQ reporting to cut instances of disagreement if indeed that's what we want to do --- need to see if that's available or needs to be added.

brucehoward-physics · 2023-11-01T20:46:27Z

Per @PetrilloAtWork request to make things less confusing (even if we still have to figure out about the differences between this and develop) I've changed the base branch to be release_v09_72_00_03 which looking at my area's git log seemed to be what the tag I was working from pointed to.

PetrilloAtWork

Looks ok, comments left mostly for C++ improval plus an error that I would prefer to be fatal.
~~Becomes approval if you think it's ok as is.~~
Ok, it's already approval because I messed up. But it's ok.

PetrilloAtWork · 2023-11-09T01:41:18Z

sbncode/CAFMaker/CAFMaker_module.cc

+      if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {
+        fBNBInfoEventMap[info.event] = info;
+      }
+      else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >


Can you put this into a function, like:

namespace { template <typename SpillInfo> double spillInfoToTimestamp(SpillInfo const& info) { return static_cast<double>(info.spill_time_s) + static_cast<double>(info.spill_time_ns)*1e-9; } }

Even better would be to add a timestamp() member function to caf::SRBNBSpillInfo and sister.

Good idea! Is the namespace { } enclosure here just a generic template of such a function or do you prefer it live in a specific namespace? Otherwise, I was thinking to just put it in caf namespace in the code.

The unnamed namespace has a specific meaning, in that its content is not visible outside the ~~translation unit~~ C++ source file where it is defined. For example, if I wrote a header file cheater.h like:

template <typename SpillInfo> double spillInfoToTimestamp(SpillInfo const& info);

and I included it in another source file:

#include "cheater.h" double const timeStamp = spillInfoToTimestamp(SpillInfo{});

this would fail linking because spillInfoToTimestamp would not be found anywhere.

[example faults: (1) for a template this should not be done in such way anyway (pretend it's not a template) and (2) the header where SpillInfo is declared needs to be included somewhere]

PetrilloAtWork · 2023-11-09T01:51:31Z

sbncode/CAFMaker/CAFMaker_module.cc

+      if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {
+        fBNBInfoEventMap[info.event] = info;
+      }
+      else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >
+                (fBNBInfoEventMap[info.event].spill_time_s+(fBNBInfoEventMap[info.event].spill_time_ns/1.0e9)) ) {
+        fBNBInfoEventMap[info.event] = info;
+      }


Minor point: optimisation to reduce lookups in the map:

Suggested change

if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {

fBNBInfoEventMap[info.event] = info;

}

else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >

(fBNBInfoEventMap[info.event].spill_time_s+(fBNBInfoEventMap[info.event].spill_time_ns/1.0e9)) ) {

fBNBInfoEventMap[info.event] = info;

}

auto const iInfo = fBNBInfoEventMap.find(info.event);

if ( iInfo==fBNBInfoEventMap.end() ) {

fBNBInfoEventMap[info.event] = info;

}

else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >

(iInfo->spill_time_s+(iInfo->spill_time_ns/1.0e9)) ) {

*iInfo = info;

}

Another approach is a bit more convoluted:

Suggested change

if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {

fBNBInfoEventMap[info.event] = info;

}

else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >

(fBNBInfoEventMap[info.event].spill_time_s+(fBNBInfoEventMap[info.event].spill_time_ns/1.0e9)) ) {

fBNBInfoEventMap[info.event] = info;

}

auto& storedInfo = fBNBInfoEventMap[info.event];

if (storedInfo.event == UINT_MAX) { // no info

storedInfo = info;

}

else if ( (info.spill_time_s+info.spill_time_ns/1.0e9) >

(storedInfo.spill_time_s+storedInfo.spill_time_ns/1.0e9)) ) {

storedInfo = info;

}

which relies on the fact that a default-constructed SpillInfo object has an event number UINT_MAX; or, merging with the other suggestion:

Suggested change

if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {

fBNBInfoEventMap[info.event] = info;

}

else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >

(fBNBInfoEventMap[info.event].spill_time_s+(fBNBInfoEventMap[info.event].spill_time_ns/1.0e9)) ) {

fBNBInfoEventMap[info.event] = info;

}

auto& storedInfo = fBNBInfoEventMap[info.event]; // creates if needed

if ((storedInfo.event == UINT_MAX) // no info

|| spillInfoToTimestamp(info) > spillInfoToTimestamp(storedInfo) // newer

{

storedInfo = std::move(info);

}

sbncode/CAFMaker/CAFMaker_module.cc

PetrilloAtWork · 2023-11-09T01:55:20Z

sbncode/CAFMaker/CAFMaker_module.cc

+    std::cout << "Found > 0 BNBInfo size and NuMIInfo size, which seems strange. Will not fill event-specific spill quality info for this event..." << std::endl;
+  }
+  else if ( fHasBNBInfo ) {
+    if ( fBNBInfoEventMap.find(evt.id().event()) == fBNBInfoEventMap.end() )


Or, more intelligible:

Suggested change

if ( fBNBInfoEventMap.find(evt.id().event()) == fBNBInfoEventMap.end() )

unsigned int const eventNo = evt.id().event();

if ( fBNBInfoEventMap.count(eventNo) == 0 )

Wait shouldn't count > 0 if fHasBNBInfo ?

That count "counts" the number of entries in the map for eventNo: there may be some information (in which case presumably fHasBNBInfo is true), but the one for eventNo might be missing, in which case that count(eventNo) returns 0.
I would call that situation a problem, but technically it is possible.

PetrilloAtWork · 2023-11-09T01:56:49Z

sbncode/CAFMaker/CAFMaker_module.cc

+    else rec.hdr.spillbnbinfo = makeSRBNBInfo(fBNBInfoEventMap.at(evt.id().event()));
+  }
+  else if ( fHasNuMIInfo ) {
+    if ( fNuMIInfoEventMap.find(evt.id().event()) == fNuMIInfoEventMap.end() )


…add another). Thanks Gianluca

brucehoward-physics · 2023-11-15T01:40:49Z

@PetrilloAtWork does what I just pushed address your idea with the count() correctly or not quite?

Tried to address other points as well, let me know if something misses the mark. Thanks for the suggestions! (Will be running a test to see that I get the same POT values or something else as with my previous test.)

PetrilloAtWork · 2023-11-17T00:28:03Z

sbncode/BeamSpillInfoRetriever/NuMIRetriever/NuMIRetriever_module.cc

  auto const & raw_data = (*raw_data_ptr);

+  // NOTE: Really we should skip the first event of each trigger type, so let's make this look at that too...
+  if ( raw_data.size()==0 ) return;


I can't believe I missed one of my favourite pet peeves!

Suggested change

if ( raw_data.size()==0 ) return;

if ( raw_data.empty() ) return;

PetrilloAtWork

I confirm the approval.

miquelnebot · 2023-11-17T18:04:57Z

@brucehoward-physics @PetrilloAtWork Is this still needed for the " current ICARUS production" or as been discussed can go into develop and into the soon coming new production release?

PetrilloAtWork · 2023-11-21T19:26:03Z

@brucehoward-physics @PetrilloAtWork Is this still needed for the " current ICARUS production" or as been discussed can go into develop and into the soon coming new production release?

I hereby summon @SFBayLaser since I definitely don't have knowledge current enough to answer.

brucehoward-physics · 2023-11-21T22:44:40Z

I'm seeing something slightly strange in my last test so probably best to not merge just yet... I'll try to figure out what's going on (or re-do the test?)...

…nyway :)

brucehoward-physics · 2023-12-07T04:44:06Z

I don't know what happened with my initial attempted retest where things didn't seem to add up but I am getting the same output POT I got the first time, so I'm back to thinking this is okay... I even added the fix for @PetrilloAtWork 's preference to use empty()

brucehoward-physics · 2025-02-21T20:16:07Z

@kjplows I see this got assigned to me. I had to go back and try to remember, but I think this PR can just be closed (🤔)

See e.g. #407

but maybe someone else here remembers if there was a reason to keep this version around?

kjplows · 2025-03-26T12:27:26Z

Hi @brucehoward-physics , since I haven't heard anyone object and #407 got merged, I am closing this PR now. Many thanks!

brucehoward-physics added 2 commits September 8, 2023 17:56

Add individual BNBInfo and NuMIInfo to SRHeader to enable saving of t…

7c887d7

…he spill quality for the spill that goes with a given event, and code supporting that.

Fix a few issues - one adds a check on PadTime before deciding if it …

ed71ae6

…wants to use this spill. The other checks not just for the first event but the first event OF A GIVEN STREAM for the numi case.

brucehoward-physics requested review from PetrilloAtWork, gputnam, jacoblarkin, jzennamo and jzettle October 16, 2023 17:05

brucehoward-physics mentioned this pull request Oct 16, 2023

Feature/howard spillquality SBNSoftware/sbnanaobj#109

Closed

brucehoward-physics changed the base branch from develop to release_v09_72_00_03 November 1, 2023 20:44

PetrilloAtWork approved these changes Nov 9, 2023

View reviewed changes

brucehoward-physics added 2 commits November 14, 2023 11:34

Updated code with some of Gianluca P's suggestions (need to test and …

97fa843

…add another). Thanks Gianluca

Second part of incorporating Gianluca's comments. Thanks

d3e5959

PetrilloAtWork reviewed Nov 17, 2023

View reviewed changes

PetrilloAtWork self-requested a review November 17, 2023 00:31

PetrilloAtWork approved these changes Nov 17, 2023

View reviewed changes

Adding to fix a pet peeve of Gianluca since I was re-testing things a…

2dd7535

…nyway :)

brucehoward-physics mentioned this pull request Jan 11, 2024

Feature/howard spillquality for develop based on j zennamo branch #407

Merged

jzennamo approved these changes Jan 17, 2024

View reviewed changes

miquelnebot added the enhancement New feature or request label Jan 18, 2024

brucehoward-physics mentioned this pull request Feb 15, 2024

Feature/howard spillquality numi analysis #425

Merged

PetrilloAtWork requested a review from JosiePaton May 13, 2024 17:51

kjplows assigned brucehoward-physics Feb 12, 2025

kjplows closed this Mar 26, 2025

github-project-automation bot moved this from Needs revisiting to Done in SBN software development Mar 26, 2025

kjplows moved this from Done to 2025 PRs in SBN software development Jan 16, 2026

-      if ( fBNBInfoEventMap.find(info.event)==fBNBInfoEventMap.end() ) {
-        fBNBInfoEventMap[info.event] = info;
-      }
-      else if ( (info.spill_time_s+(info.spill_time_ns/1.0e9)) >
-                (fBNBInfoEventMap[info.event].spill_time_s+(fBNBInfoEventMap[info.event].spill_time_ns/1.0e9)) ) {
-        fBNBInfoEventMap[info.event] = info;
-      }
+      auto& storedInfo = fBNBInfoEventMap[info.event];
+      if (storedInfo.event == UINT_MAX) { // no info
+        storedInfo = info;
+      }
+      else if ( (info.spill_time_s+info.spill_time_ns/1.0e9) >
+                (storedInfo.spill_time_s+storedInfo.spill_time_ns/1.0e9)) ) {
+        storedInfo = info;
+      }

	if ( fBNBInfoEventMap.find(evt.id().event()) == fBNBInfoEventMap.end() )
	unsigned int const eventNo = evt.id().event();
	if ( fBNBInfoEventMap.count(eventNo) == 0 )

	if ( raw_data.size()==0 ) return;
	if ( raw_data.empty() ) return;

Feature/howard spillquality #390

Feature/howard spillquality #390

Uh oh!

Conversation

brucehoward-physics commented Oct 16, 2023

Uh oh!

brucehoward-physics commented Oct 16, 2023

Uh oh!

brucehoward-physics commented Nov 1, 2023

Uh oh!

PetrilloAtWork left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brucehoward-physics commented Nov 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

miquelnebot commented Nov 17, 2023

Uh oh!

PetrilloAtWork commented Nov 21, 2023

Uh oh!

brucehoward-physics commented Nov 21, 2023

Uh oh!

brucehoward-physics commented Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brucehoward-physics commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kjplows commented Mar 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

PetrilloAtWork left a comment •

edited

Loading

brucehoward-physics commented Dec 7, 2023 •

edited

Loading

brucehoward-physics commented Feb 21, 2025 •

edited

Loading