ADD: New feature to write different data types into roa file by zoglauer · Pull Request #213 · cositools/nuclearizer

zoglauer · 2026-09-22T15:51:00Z

** NEEDS YET ANOTHER MEGALIB UPDATE ***

First version to handle multiple data types in the roa file, e.g.:

TYPE ROA
UF UH doublesidedstrip adc-tac-flags
UF UC voxel3d adc-flags

SE
ID 1
TI 1771332026.007614416
UH 0 64 h 1738 0 5
UC 4 2536 1

Lots of changes in MEGAlib and nuclearizer were necessary to achieve this - and I do not have a data set to fully test it yet.
Please review carefully.

fhagemann · 2026-09-22T15:52:32Z

This will take me some time to review. I can look at this after Wednesday.

zoglauer · 2026-09-22T16:10:25Z

This supersedes PR #209, PR #156, and issue #66

fhagemann

Nice PR and thanks for all the work :)

In this review, I only looked at the code, without running it.
Most of my comments are minor / code-style questions.

The biggest questions would be:

would we still be able to read "old" ROA files with UF doublesidedstrip adc_tac_flags or will this PR be breaking in the sense that we MUST add UH as second token: UF UC doublesidedstrip adc-tac-flags

This looks like it might allow to use ROA for simulation files including both the germanium strip detectors and the BGO shields!

I will try playing a bit around with this using simulated data files, when reading/parsing ROA files, and will let you know how that goes in my final review! But this looks like it might close #66 and be a more thorough fix than what #156 was!

fhagemann · 2026-09-24T03:05:22Z

+UF UH doublesidedstrip adc-tac-flags
+UF UC voxel3d adc-flags


I see: my understanding of this is that the UF lines now have an additional token after UF denoting which two letters are used in the rest of the file to denote a hit in the given detector type.

My question would be:

do old ROA file formats still work (if the first line was UF doublesidedstrip adc-tac-flags, would it default to UH, or will we not be able to read "older" ROA files after merging this PR)?

Or in other words: If we do not have any crystal data in this file, do we need this update?

fhagemann · 2026-09-24T03:08:23Z

+    for (unsigned int d = 0; d < CrystalData.size(); ++d) {
+      if (d != 0) CrystalType += "-";
+      CrystalType += CrystalData[d];
+    }


Just curious: what made you transition from underscores _ to dashes - to separate the m_Type names in the ROA UF line?

fhagemann · 2026-09-24T03:09:31Z

+      return false;
+    }
+    if (CrystalType != "") {
+      m_RoaFileFormat.AddReadOutUnit("UC", "voxel3d", CrystalType);


Does this also need a if (m_RoaFileFormat.AddReadOutUnit("UC", "voxel3d", CrystalType) == false) + verbosity output? Will this ever NOT work?

fhagemann · 2026-09-24T03:10:26Z

  new MXmlNode(Node, "RoaWithTACs", m_RoaWithTACs);
  new MXmlNode(Node, "RoaWithEnergies", m_RoaWithEnergies);
  new MXmlNode(Node, "RoaWithTimings", m_RoaWithTimings);
+  new MXmlNode(Node, "RoaWithTemperatures", m_RoaWithTemperatures);


This is added back in on purpose (relevant for the shields)?

fhagemann · 2026-09-24T03:17:22Z

I checked this file locally:
it also has the UF UC voxel3d adc-flags line at the top (like the 542-1 tests), but not a single line later that starts with UC (which makes sense because we did not take data with shields yet).

Once we have HDF5 files with both StripHits and CrystalHits, we might want to update/add the unit tests.

fhagemann · 2026-09-24T03:24:14Z


-  S<<"UH "
-   <<m_ReadOutElement->GetDetectorID()<<" ";
+  S<<"UC "<<m_ReadOutElement->ToParsableString()<<" ";


If I understand correctly, using ToParsableString will now print

UC DetectorID CrystalID VoxelXID VoxelYID VoxelZID .....

instead of just

UH DetectorID .....

fhagemann · 2026-09-24T03:30:17Z

@@ -476,10 +477,8 @@ bool MReadOutAssembly::Parse(MString& Line, int Version)
      return false;
    }
  }
-  if (Line.BeginsWith("BD")) {
-    // Mark the event as filtered out
-    m_FilteredOut = true;
-    return true;
+  if (Line.BeginsWith("BD") || Line.BeginsWith("QA") || Line.BeginsWith("PQ")) {
+    return ParseBDFlags(Line);
  }

  // Everything else goes to the tolerant base parser, which also consumes unrecognized lines and returns true
@@ -547,9 +546,9 @@ bool MReadOutAssembly::GetNextFromDatFile(MFile& F)
      } else {
        delete sh;
      }
-    } else if (Line.BeginsWith("BD")) {
+    } else if (Line.BeginsWith("BD") || Line.BeginsWith("QA") || Line.BeginsWith("PQ")) {
      EventRead = true;
-      SetFilteredOut(true);
+      ParseBDFlags(Line);
    }


Ignore if this should become part of future clean-up, but should this be Line.BeginsWith(...) == true instead of just Line.BeginsWith(...).

Not sure if there is a coding convention for evaluating boolean values in if-statements for nuclearizer.

fhagemann · 2026-09-24T03:30:44Z

+  // The setters ignore an empty text
+  if (Texts.empty() == true) Texts.push_back("");
+
+  if (Line.BeginsWith("BD")) {


Suggested change

if (Line.BeginsWith("BD")) {

if (Line.BeginsWith("BD") == true) {

?

fhagemann · 2026-09-24T03:31:10Z

+    return true;
+  }
+
+  if (Line.BeginsWith("QA")) {


Suggested change

if (Line.BeginsWith("QA")) {

if (Line.BeginsWith("QA") == true) {

fhagemann · 2026-09-24T03:33:20Z

+    return true;
+  }
+
+  return false;


You're checking with Line.BeginsWith(...) in Parse already, but should we add a warning/error message here, just to be sure, in case it should be used somewhere else where we don't have that check implemented?

Suggested change

return false;

if (g_Verbosity >= c_Error) cout<<... ?

return false;

fhagemann · 2026-09-24T04:28:36Z

** NEEDS YET ANOTHER MEGALIB UPDATE ***

First version to handle multiple data types in the roa file, e.g.:

TYPE ROA
UF UH doublesidedstrip adc-tac-flags
UF UC voxel3d adc-flags

SE
ID 1
TI 1771332026.007614416
UH 0 64 h 1738 0 5
UC 4 2536 1

Lots of changes in MEGAlib and nuclearizer were necessary to achieve this - and I do not have a data set to fully test it yet.
Please review carefully.

I created an ROA file based on a cosima simulation that ran through the DEE + energy calibration + TAC calibration, this in an example event I get:

SE
ID 16556
TI 3778335.577032771
UH 0 10 l 1839 10648 42.433 4824 
UH 0 9 l 1587 10603 1.87772 4801.5 
UH 0 11 l 1665 12723 0.70402 5861.5 
UH 0 37 h 1805 10727 40.3225 4863.5 
UH 0 36 h 1592 10605 0.719259 4802.5 
UC X0 4294967284 4294967284 4294967284 4294967284 2026 0 
PQ

(so also the crystal ID and voxelX/Y/Z, that right now always seems to be 4294967284, at least in my cosima/DEE simulations)

fhagemann · 2026-09-24T04:39:36Z

+    } else if (Flag == "No hits") {
+      // Written by StreamRoa for events without hits, nothing to set


Not sure if this is relevant, but I'm losing all events with BD No hits when running my own ROARoundTrip (or it can also be that I'm doing something wrong and this is to be blamed on my settings):

I start with an ROA file (created from cosima + DEE, saved to ROA) which contains:
SE ID 16557 TI 3778504.201794552 BD No hits PQ SE ID 16558 TI 3778761.823754169 BD No hits PQ

I run nuclearizer only reading the ROA file and writing the results into a new ROA file.

Those events (ID 16557 and ID 16558) do not appear in this new ROA file.

fhagemann · 2026-09-24T04:51:01Z

+  const MString Input = Header +
+    "SE\nID 1\nTI 1.500000000\n"
+    "UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"
+    "UH 0 49 h 1780 10251 33.25 11.5 0 -\n"
+    "UC BGO1 2 0 1 3 812 511.5 0 3\n"
+    "SE\nID 2\nTI 2.250000000\n"
+    "UC BGO2 0 1 1 1 900 600.25 0 -\n"
+    "EN\n";
+  // The writer adds a blank line before the header, a PQ line to each event, and a blank line at the end
+  const MString Expected = "\n" + Header +
+    "SE\nID 1\nTI 1.500000000\n"
+    "UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"
+    "UH 0 49 h 1780 10251 33.25 11.5 0 -\n"
+    "UC BGO1 2 0 1 3 812 511.5 0 3\n"
+    "PQ\n"
+    "SE\nID 2\nTI 2.250000000\n"
+    "UC BGO2 0 1 1 1 900 600.25 0 -\n"
+    "PQ\n"
+    "EN\n\n";


I can reproduce losing events with BD No hits (plus their event IDs and TI values in the unit tests).

I can add a BD No hits event to the Input and I do not need to modify the Expected for the tests to still pass:

Suggested change

const MString Input = Header +

"SE\nID 1\nTI 1.500000000\n"

"UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"

"UH 0 49 h 1780 10251 33.25 11.5 0 -\n"

"UC BGO1 2 0 1 3 812 511.5 0 3\n"

"SE\nID 2\nTI 2.250000000\n"

"UC BGO2 0 1 1 1 900 600.25 0 -\n"

"EN\n";

// The writer adds a blank line before the header, a PQ line to each event, and a blank line at the end

const MString Expected = "\n" + Header +

"SE\nID 1\nTI 1.500000000\n"

"UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"

"UH 0 49 h 1780 10251 33.25 11.5 0 -\n"

"UC BGO1 2 0 1 3 812 511.5 0 3\n"

"PQ\n"

"SE\nID 2\nTI 2.250000000\n"

"UC BGO2 0 1 1 1 900 600.25 0 -\n"

"PQ\n"

"EN\n\n";

const MString Input = Header +

"SE\nID 1\nTI 1.500000000\n"

"UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"

"UH 0 49 h 1780 10251 33.25 11.5 0 -\n"

"UC BGO1 2 0 1 3 812 511.5 0 3\n"

"SE\nID 2\nTI 2.250000000\n"

"UC BGO2 0 1 1 1 900 600.25 0 -\n"

"SE\nID 3\n TI 4.000000000\n" // added this line

"BD No hits\n" // and this line

"EN\n";

// The writer adds a blank line before the header, a PQ line to each event, and a blank line at the end

const MString Expected = "\n" + Header +

"SE\nID 1\nTI 1.500000000\n"

"UH 0 41 l 4053 10452 59.5 12.25 4 1;2\n"

"UH 0 49 h 1780 10251 33.25 11.5 0 -\n"

"UC BGO1 2 0 1 3 812 511.5 0 3\n"

"PQ\n"

"SE\nID 2\nTI 2.250000000\n"

"UC BGO2 0 1 1 1 900 600.25 0 -\n"

"PQ\n"

"EN\n\n";

Is it ever helpful to keep those events (ID and TI), even if they have no hits in them?

ADD: New feature to write different data types into roa file

75eddfa

zoglauer requested review from ckierans and fhagemann September 22, 2026 15:51

fhagemann linked an issue Sep 22, 2026 that may be closed by this pull request

Add all options we can write a roa file also in the reader #66

Open

fhagemann reviewed Sep 24, 2026

View reviewed changes

	if (Line.BeginsWith("BD")) {
	if (Line.BeginsWith("BD") == true) {

	if (Line.BeginsWith("QA")) {
	if (Line.BeginsWith("QA") == true) {

	return false;
	if (g_Verbosity >= c_Error) cout<<... ?
	return false;

		} else if (Flag == "No hits") {
		// Written by StreamRoa for events without hits, nothing to set

Conversation

zoglauer commented Sep 22, 2026

Uh oh!

fhagemann commented Sep 22, 2026

Uh oh!

zoglauer commented Sep 22, 2026

Uh oh!

fhagemann left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

fhagemann commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants