add first NuGraph2 info to CAF #137

cerati · 2025-04-11T03:40:46Z

This PR adds a few NuGraph-related variables to CAFs, along the lines of what presented on docdb 40585. Once SBNSoftware/icaruscode#815 is merged the needed data products can be produced, but it won't break CAF making if they are not there. The related SBNSoftware/sbncode#532 is also being made.

brucehoward-physics

Seems pretty uncontroversial, but I do have a few suggestions for collapsing the checksum versions.

brucehoward-physics · 2025-05-05T03:25:37Z

sbnanaobj/StandardRecord/classes_def.xml

+   <version ClassVersion="23" checksum="2124497341"/>
+   <version ClassVersion="22" checksum="4187618741"/>


I believe you can just collapse these to ClassVersion 22 with the latest checksum no?

brucehoward-physics · 2025-05-05T03:25:46Z

sbnanaobj/StandardRecord/classes_def.xml

+   <version ClassVersion="17" checksum="135310109"/>
+   <version ClassVersion="16" checksum="1684266144"/>


…nto feature/cerati_ng2caf

cerati · 2025-05-05T18:08:32Z

@brucehoward-physics Suggestions implemented. Thanks

PetrilloAtWork

Left a few comments.
Only two, really.
A shame.

PetrilloAtWork · 2025-05-07T00:33:58Z

sbnanaobj/StandardRecord/SRPFP.h

+  int   ng_sem_cat;    ///< NuGraph semantic category with largest hit fraction
+  float ng_mip_frac; ///< Fraction of PFP hits labeled by NuGraph as MIP
+  float ng_hip_frac; ///< Fraction of PFP hits labeled by NuGraph as HIP
+  float ng_shr_frac; ///< Fraction of PFP hits labeled by NuGraph as shower
+  float ng_mhl_frac; ///< Fraction of PFP hits labeled by NuGraph as Michel
+  float ng_dif_frac; ///< Fraction of PFP hits labeled by NuGraph as diffuse
+  float ng_bkg_frac; ///< Fraction of PFP hits labeled by NuGraph as background


I recommend to enclose this group of data members into their own class, like in SRCNNScore (e.g. SRNGScore) or, even better from my point of view, use and extend SRCNNScore itself (half the categories are the same), with the unsupported categories defaulting to 0.0 (and nclusters to -5).

PetrilloAtWork · 2025-05-07T00:35:57Z

sbnanaobj/StandardRecord/SRPFP.h


  SRCNNScore cnnscore; // CNN scores for this PFP

+  int   ng_sem_cat;    ///< NuGraph semantic category with largest hit fraction


Consider an enum type instead of a bare int.
And consider very well having at very least an enum to describe the possible categories rather than using integer values in the code.

kjplows · 2025-05-07T22:03:22Z

trigger build

FNALbuild · 2025-05-07T22:06:16Z

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

FNALbuild · 2025-05-07T22:06:20Z

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

FNALbuild · 2025-05-07T22:07:29Z

❌ 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 · 2025-05-07T22:07:32Z

❌ 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 · 2025-05-07T22:19:08Z

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof -- 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 · 2025-05-07T22:24:37Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored failure for unit_test -- 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

…nto feature/cerati_ng2caf

PetrilloAtWork

Several formal C++ fixes requested.
Also, I believe the new class should be added to classes_def.xml.
As usual, do not accept suggested documentation without checking what I wrote, that may be badly written, misleading or plain wrong.

PetrilloAtWork · 2025-05-08T00:23:18Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+  class SRNuGraphScore {
+  public:
+    SRNuGraphScore();
+    ~SRNuGraphScore() {}


Remove this destructor: it's useless and sometimes deleterious.

Suggested change

~SRNuGraphScore() {}

PetrilloAtWork · 2025-05-08T00:28:08Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+    float mip_frac;
+    float hip_frac;
+    float shr_frac;
+    float mhl_frac;
+    float dif_frac;
+    float bkg_frac;


Initialize the data members inline.

Add in comment what these are supposed to be. Maybe something like:

Suggested change

float mip_frac;

float hip_frac;

float shr_frac;

float mhl_frac;

float dif_frac;

float bkg_frac;

static constexpr float kUnassigned = std::numeric_limits<float>::signaling_NaN();

float mip_frac = kUnassigned; ///< Fraction of hits that are most likely to be `MIP`.

float hip_frac = kUnassigned; ///< Fraction of hits that are most likely to be `HIP`.

float shr_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Shower`.

float mhl_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Michel`.

float dif_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Diffuse`.

float bkg_frac = kUnassigned; ///< Fraction of hits that are most likely to be none of the above.

(for this example you also have to #include <limits> here instead of the implementation file).
Here I am defining kUnassigned just for convenience. It may be private, it may be just spelled out — your choice.

If you're going to use signaling NaN why not just use this ;)

https://github.com/SBNSoftware/sbnanaobj/blob/develop/sbnanaobj/StandardRecord/SRConstants.h#L8

PetrilloAtWork · 2025-05-08T00:34:57Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+      Diffuse = 4
+    };
+
+    int sem_cat;


Initialize the data members inline ([CF-155]). It used to be not the CAF standard, but it is going to become under by guard. Too many times members are added that are left uninitialised, and this helps a lot against it.

Add in comment what these are supposed to be. For example:

Suggested change

int sem_cat;

int sem_cat = NuGraphCategory::Unset; ///< The category of the majority of the hits.

Finally, in CAF the pattern used to distinguish between default-constructed and positively set as invalid/unknown; consider to use -5 instead of Unset for this construction (yes, it's not in the enum, and it might be ok considering that it represents a "logic error" state where the object was not appropriately initialised).
I am even not sure whether I would do that, but as CAF maintainer I am bound to explain that this is the custom ([NA-01]).

PetrilloAtWork · 2025-05-08T00:35:37Z

sbnanaobj/StandardRecord/SRPFP.h


  SRCNNScore cnnscore; // CNN scores for this PFP

+  SRNuGraphScore ngscore; // NuGraph scores for this PFP


Doxy it!

Suggested change

SRNuGraphScore ngscore; // NuGraph scores for this PFP

SRNuGraphScore ngscore; ///< NuGraph scores for this PFP

(if you can, also the cnnscore one above)

PetrilloAtWork · 2025-05-08T00:39:05Z

sbnanaobj/StandardRecord/classes_def.xml

+  <class name="caf::SRPFP" ClassVersion="17">
+   <version ClassVersion="17" checksum="2252062519"/>
+   <version ClassVersion="16" checksum="135310109"/>


Please compress this into a single checksum:

Suggested change

<class name="caf::SRPFP" ClassVersion="17">

<version ClassVersion="17" checksum="2252062519"/>

<version ClassVersion="16" checksum="135310109"/>

<class name="caf::SRPFP" ClassVersion="16">

<version ClassVersion="16" checksum="2252062519"/>

sbnanaobj/StandardRecord/classes_def.xml

PetrilloAtWork · 2025-05-08T00:56:47Z

sbnanaobj/StandardRecord/SRNuGraphScore.cxx

+#include <limits>
+
+namespace caf
+{
+  SRNuGraphScore::SRNuGraphScore() :
+    sem_cat(NuGraphCategory::Unset),
+    mip_frac(std::numeric_limits<float>::signaling_NaN()),
+    hip_frac(std::numeric_limits<float>::signaling_NaN()),
+    shr_frac(std::numeric_limits<float>::signaling_NaN()),
+    mhl_frac(std::numeric_limits<float>::signaling_NaN()),
+    dif_frac(std::numeric_limits<float>::signaling_NaN()),
+    bkg_frac(std::numeric_limits<float>::signaling_NaN())
+  {
+  }
+}


When following the recommendations on the constructor (below), not much is left here.

Suggested change

#include <limits>

namespace caf

{

SRNuGraphScore::SRNuGraphScore() :

sem_cat(NuGraphCategory::Unset),

mip_frac(std::numeric_limits<float>::signaling_NaN()),

hip_frac(std::numeric_limits<float>::signaling_NaN()),

shr_frac(std::numeric_limits<float>::signaling_NaN()),

mhl_frac(std::numeric_limits<float>::signaling_NaN()),

dif_frac(std::numeric_limits<float>::signaling_NaN()),

bkg_frac(std::numeric_limits<float>::signaling_NaN())

{

}

}

(in fact, the only reason to include a .cxx is to make sure that the header is compiled and checked)

sbnanaobj/StandardRecord/SRNuGraphScore.h

PetrilloAtWork · 2025-05-08T00:59:35Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+
+  class SRNuGraphScore {
+  public:
+    SRNuGraphScore();


Following the recommendation on the in-class initialization, this is not needed:

Suggested change

SRNuGraphScore();

PetrilloAtWork · 2025-05-08T01:02:12Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+      Unset = -1,
+      Mip = 0,
+      Hip = 1,
+      Shower = 2,
+      Michel = 3,
+      Diffuse = 4


Shouldn't MIP and HIP go all capital?
Also have some comment, e.g. to explain that MIP and HIP refer to a track-like object, what is "diffuse" meaning, that Michel is not the pianist...

Suggested change

Unset = -1,

Mip = 0,

Hip = 1,

Shower = 2,

Michel = 3,

Diffuse = 4

Unset = -1, ///< Unset/unknown/undecided/ category.

Mip = 0, ///< Track from a minimum-ionizing particle.

Hip = 1, ///< Track from a high ionization particle.

Shower = 2, ///< Particle shower.

Michel = 3, ///< Michel electron (from muon decay).

Diffuse = 4 ///< Diffuse background.

Still not sure how to define Diffuse.

PetrilloAtWork

Just one fix to the documentation left.

PetrilloAtWork · 2025-05-08T02:41:36Z

sbnanaobj/StandardRecord/SRNuGraphScore.h

+/**
+ * @brief Categorization of the object/PFP by NuGraph.
+ *
+ * This object summarizes the results from running NuGraph over hits in a slice
+ * (see e.g. [https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=40585](SBN DocDB 40585).
+ *
+ * The semantic category is the one that most hits belong to.
+ * The fractions describe the categorization of the hits in the object/PFP.
+ */
+namespace caf {
+
+  class SRNuGraphScore {


The comment needs to be just before the object it's commenting about (in this code, the documentation would have been attached to the caf namespace):

Suggested change

/**

* @brief Categorization of the object/PFP by NuGraph.

*

* This object summarizes the results from running NuGraph over hits in a slice

* (see e.g. [https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=40585](SBN DocDB 40585).

*

* The semantic category is the one that most hits belong to.

* The fractions describe the categorization of the hits in the object/PFP.

*/

namespace caf {

class SRNuGraphScore {

namespace caf {

/**

* @brief Categorization of the object/PFP by NuGraph.

*

* This object summarizes the results from running NuGraph over hits in a slice

* (see e.g. [https://sbn-docdb.fnal.gov/cgi-bin/sso/ShowDocument?docid=40585](SBN

* DocDB 40585).

*

* The semantic category is the one that most hits belong to.

* The fractions describe the categorization of the hits in the object/PFP.

*/

class SRNuGraphScore {

PetrilloAtWork

Thank you, Giuseppe.

kjplows · 2025-05-08T05:18:05Z

trigger build

FNALbuild · 2025-05-08T05:21:03Z

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

FNALbuild · 2025-05-08T05:21:10Z

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

FNALbuild · 2025-05-08T05:22:09Z

❌ 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 · 2025-05-08T05:22:10Z

❌ 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 · 2025-05-08T05:33:52Z

⚠️ CI build for SBND Warning at phase ci_tests SBND on slf7 for e26:prof -- 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 · 2025-05-08T06:00:18Z

⚠️ CI build for ICARUS Warning at phase ci_tests ICARUS on slf7 for e26:prof - ignored failure for unit_test -- 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

add first NuGraph2 info to CAF

82f8d5a

cerati added the enhancement New feature or request label Apr 11, 2025

cerati requested review from PetrilloAtWork and brucehoward-physics April 11, 2025 03:40

cerati mentioned this pull request Apr 11, 2025

Feature/cerati ng2caf SBNSoftware/sbncode#532

Merged

kjplows added this to SBN software development Apr 17, 2025

kjplows moved this to Open pull requests in SBN software development Apr 17, 2025

Merge branch 'develop' into feature/cerati_ng2caf

0a7a220

brucehoward-physics approved these changes May 5, 2025

View reviewed changes

cerati added 2 commits May 5, 2025 13:06

collapse class versions

3bd4a13

Merge branch 'feature/cerati_ng2caf' of github.com:cerati/sbnanaobj i…

bd248a0

…nto feature/cerati_ng2caf

Merge branch 'develop' into feature/cerati_ng2caf

4bbecb2

PetrilloAtWork reviewed May 7, 2025

View reviewed changes

kjplows moved this from Open pull requests to Testing in SBN software development May 7, 2025

cerati added 2 commits May 7, 2025 18:59

address GP's comments

69a2cc5

Merge branch 'feature/cerati_ng2caf' of github.com:cerati/sbnanaobj i…

9fcfc78

…nto feature/cerati_ng2caf

PetrilloAtWork requested changes May 8, 2025

View reviewed changes

cerati added 2 commits May 7, 2025 20:29

address GP's comments

2d92fdc

address GP's comments

75f2031

PetrilloAtWork requested changes May 8, 2025

View reviewed changes

address GP's comments

b4a26ad

PetrilloAtWork approved these changes May 8, 2025

View reviewed changes

kjplows moved this from Testing to To merge in SBN software development May 8, 2025

Merge branch 'develop' into feature/cerati_ng2caf

4fb7122

kjplows merged commit 8e68ee8 into SBNSoftware:develop May 8, 2025

github-project-automation bot moved this from To merge to Done in SBN software development May 8, 2025

kjplows moved this from Done to Recently done in SBN software development May 8, 2025

kjplows moved this from Recently done to Done in SBN software development Jul 10, 2025

		<version ClassVersion="23" checksum="2124497341"/>
		<version ClassVersion="22" checksum="4187618741"/>

		<version ClassVersion="17" checksum="135310109"/>
		<version ClassVersion="16" checksum="1684266144"/>


		SRCNNScore cnnscore; // CNN scores for this PFP

		int ng_sem_cat; ///< NuGraph semantic category with largest hit fraction

-    float mip_frac;
-    float hip_frac;
-    float shr_frac;
-    float mhl_frac;
-    float dif_frac;
-    float bkg_frac;
+    static constexpr float kUnassigned = std::numeric_limits<float>::signaling_NaN();
+    float mip_frac = kUnassigned; ///< Fraction of hits that are most likely to be `MIP`.
+    float hip_frac = kUnassigned; ///< Fraction of hits that are most likely to be `HIP`.
+    float shr_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Shower`.
+    float mhl_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Michel`.
+    float dif_frac = kUnassigned; ///< Fraction of hits that are most likely to be `Diffuse`.
+    float bkg_frac = kUnassigned; ///< Fraction of hits that are most likely to be none of the above.

	int sem_cat;

	int sem_cat = NuGraphCategory::Unset; ///< The category of the majority of the hits.


		SRCNNScore cnnscore; // CNN scores for this PFP

		SRNuGraphScore ngscore; // NuGraph scores for this PFP

	SRNuGraphScore ngscore; // NuGraph scores for this PFP
	SRNuGraphScore ngscore; ///< NuGraph scores for this PFP

add first NuGraph2 info to CAF #137

add first NuGraph2 info to CAF #137

Uh oh!

Conversation

cerati commented Apr 11, 2025 • edited by PetrilloAtWork Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brucehoward-physics 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!

cerati commented May 5, 2025

Uh oh!

PetrilloAtWork 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!

kjplows commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

FNALbuild commented May 7, 2025

Uh oh!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork May 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

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!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

PetrilloAtWork May 8, 2025 • 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!

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Uh oh!

kjplows commented May 8, 2025

Uh oh!

FNALbuild commented May 8, 2025

Uh oh!

FNALbuild commented May 8, 2025

Uh oh!

cerati commented Apr 11, 2025 •

edited by PetrilloAtWork

Loading

PetrilloAtWork May 8, 2025 •

edited

Loading

PetrilloAtWork May 8, 2025 •

edited

Loading