Feature/howard v09370103p01 nuid features #44

brucehoward-physics · 2022-02-11T18:36:38Z

Updated PR that uses v09_37 series as the base. Requires changes in larpandoracontent to be useful.

…Slice

PetrilloAtWork

I have requested one change (removal of the destructor).
Other suggestions are up to the author.
One more generic one is that the recommended name convention is using the drinking camel-case ([NV-05]), e.g. nuTotHits (instead of nutothits). I recommend that convention to be adopted, but I don't understand most of the names so I don't know if they are... camelcaseable.

PetrilloAtWork · 2022-02-16T07:51:03Z

sbnanaobj/StandardRecord/SRNuID.h

+  {
+  public:
+    SRNuID();
+    virtual ~SRNuID();


Given that there is no virtual method in this data class, there is no reason for it to be polymorphic, so the destructor should not be virtual. Hence, there should be no destructor at all ([CF-151]).
So I request the destructor to be removed.

PetrilloAtWork · 2022-02-16T07:53:25Z

sbnanaobj/StandardRecord/SRNuID.h

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


I recommend the practice of assigning values to the data members directly rather than via a constructor
([CF-156]).
That habit makes it easier not to forget to initialize a data field when adding it.

Is the recommendation to explicitly write

float nufspfos { std::numeric_limits<float>::signaling_NaN() }; //!< NuNFinalStatePfos feature in NeutrinoID

etc. and remove the setting in the constructor? Just checking that I understand the comment and convention correctly. (Other SR code that I saw uses the constructor-setting.)

I would use = there instead of {}, but I admit I have no idea of the distinction, if any, between them. = just seems more intuitive to me.
See also this issue #24 in the long tradition of heaping extra work on people who ask questions. It would probably be a good idea to get a kSignalingNaN or whatever before we totally uglify the headers.

I confirm that, beyond the choice of syntax, the recommendation is the one @brucehoward-physics described.
We do have a guideline ([CF-060]) suggesting the use of braces. That is intended as a tie-breaker in case your mind is stuck on a tie.

Some differences between = and {} syntax (whose nickname "universal initialization" betrays its ambition) for simple type data member initialization are that the latter will complain about "narrowing" (for example if you try to assign a double value to a float variable), and that it allows explicit default initialization, for example:

unsigned short int fNADC {}; short int fADC[16] {};

will initialize fNADC to 0 and all fADC to 0 too (with no initializer, both are undefined). You can obtain the same effect with = 0 and = { 0 } respectively.
Not much difference for simple types, then. When object data members are involved, braces are a superior option (but usually equivalent to parentheses, except for noticeable exceptions like std::vector<int> and for its role in the most vexing parse).
I try to use braces mostly because since it's newer syntax it makes me look smarter. But my official excuse is that it's more uniform so I can usually manage to have only one type of syntax (available: int a = 0;, int a (0);, int a { 0 };, int a = { 0 };; so yeah, {} is the new universal standard).

Okay - have pushed commits which add a central kSignalingNaN and applies it to SRSlice and SRNuID (I didn't touch the other StandardRecord objects).

I have it set up to use braces. There are some items which are not seemingly not being forced to initialize in SRSlice. Shall I add an empty {} here too?

PetrilloAtWork · 2022-02-16T07:55:44Z

sbnanaobj/StandardRecord/SRNuID.cxx

+  SRNuID::~SRNuID(){  }
+


Useless, sometimes deleterious ([CF-151]): please remove (see later).

PetrilloAtWork · 2022-02-16T07:56:24Z

sbnanaobj/StandardRecord/SRNuID.cxx

+#include "sbnanaobj/StandardRecord/SRNuID.h"
+
+#include <limits>
+#include <climits>


I don't think climits is needed here.

Suggested change

#include <climits>

believe this is now ✅ - I mean this particular change was made but now I also made changes in SRSlice, in the process of addressing other comments.

PetrilloAtWork · 2022-02-16T07:58:44Z

sbnanaobj/StandardRecord/SRNuID.h

+#ifndef SRNUID_H
+#define SRNUID_H


I suggest having a definition name less easy to collide with, e.g.

Suggested change

#ifndef SRNUID_H

#define SRNUID_H

#ifndef SBNANAOBJ_STANDARDRECORD_SRNUID_H

#define SBNANAOBJ_STANDARDRECORD_SRNUID_H

(if changed, also change the matching #endif accordingly).

I followed what I saw in other Standard Record objects but no harm in changing this! Will do.

PetrilloAtWork · 2022-02-16T07:59:40Z

sbnanaobj/StandardRecord/SRNuID.cxx

+////////////////////////////////////////////////////////////////////////
+// \file    SRNuID.cxx
+// \brief   SRNuID object for Neutrino ID score features (MVA inputs). This SR code copied/based on other SR objects.
+// \author  $Author: howard@fnal.gov


$Author is a remnant of CVS of no meaning.

Suggested change

// \author $Author: howard@fnal.gov

// \author howard@fnal.gov

PetrilloAtWork · 2022-02-16T07:59:51Z

sbnanaobj/StandardRecord/SRNuID.h

+////////////////////////////////////////////////////////////////////////
+// \file    SRNuID.h
+// \brief   SRNuID object for Neutrino ID score features (MVA inputs). This SR code copied/based on other SR objects.
+// \author  $Author: howard@fnal.gov


Suggested change

// \author $Author: howard@fnal.gov

// \author howard@fnal.gov

cjbacchus · 2022-02-16T13:10:55Z

@PetrilloAtWork we have a conflicting convention of all-lowercase field names in StandardRecord. I'm not sure if that got documented in the guidelines anywhere.

henrylay97

All good! I know this has been discussed before so I'll just throw my two pence in here and say I preferred the capitalisation in the names of the variables. A couple are quite difficult to parse now and things like nufspfos or nusps aren't particularly obvious if you haven't got the pandora tools open in front of you!

brucehoward-physics · 2022-02-16T15:35:26Z

Thanks for the reviews!

I'm happy to follow whatever the convention is. I'll note that in general these things are following what I saw in other Standard Record object(s) so I think perhaps several conventions are being broken in Standard Record 😅 .

With the names, I adopted Chris's suggestion to use lower case names in the Standard Record. I originally had longer, camel-cased names.

…nalingNaN to centralize the signaling NaN (right now just used in SRSlice and SRNuID), could be extended further.

PetrilloAtWork

Two more language comments and a question for @cjbackhouse.
Nothing show-stopping though.

PetrilloAtWork · 2022-02-16T20:43:49Z

sbnanaobj/StandardRecord/SRNuID.cxx

+  SRNuID::SRNuID()
+  {  }
+


Given its emptiness, the constructor can be removed, unless there is the explicit intention of forbidding aggregate initialization (which I would not forbid).
So I recommend removing the constructor.

Sorry I missed your comments before finalizing my statement below. There is no intent from me here, but other Standard Record objects have empty constructors apart from variable assignment. Does this carry the same impact w.r.t. aggregate initialization? If it does then I defer to @cjbackhouse and/or @FernandaPsihas to add their opinion. If not, then I guess these could be removed.

Yeah, no opinion either way about empty constructors.

PetrilloAtWork · 2022-02-16T20:55:12Z

sbnanaobj/StandardRecord/SRHelper.h

+
+namespace caf
+{
+  const float kSignalingNaN = std::numeric_limits<float>::signaling_NaN();


I recommend setting it constexpr:

Suggested change

const float kSignalingNaN = std::numeric_limits<float>::signaling_NaN();

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

This other one is more for @cjbackhouse: is it ok to tie the name kSignalingNaN to a float value? do you foresee an use for the double counterpart?

Updated! - Only tested in that it builds.

Hmm. I do indeed think that we might one day want a double version (I normally resist doubles in the CAFs, but there are potentially legitimate reasons for one). If you assign this into a double, does the double end up NaN too, or not?
Since double will be so much rarer I think having a clunkier name like kSignalingNaNDouble for it would be ok.

PetrilloAtWork · 2022-02-16T20:56:51Z

sbnanaobj/StandardRecord/SRSlice.cxx

-    nu_pdg(INT_MIN),
-    nu_score(std::numeric_limits<float>::signaling_NaN()),
-    self(INT_MIN)
+  SRSlice::SRSlice()


Same comment as for the other class: an empty default constructor impedes some language features, and unless that is intended (in which case it would be good to add a comment stating it), the empty constructor is better gone.

Suggested change

SRSlice::SRSlice()

brucehoward-physics · 2022-02-16T21:20:33Z

I believe I have addressed most of the comments and tested all but the very last (pretty simple) commit on a single event, confirming at least that things ran and the SRNuID values in the output CAF file are set the same as they were when I started this PR. Remaining items I am currently thinking of, please note if I'm missing something:

On naming - There are conflicting requests, so I'm not sure how to proceed.
On directly assigning values - I have set up the declaration of values in the header directly for SRSlice and SRNuID (see also point 3) for things that were being assigned in the constructor. I have a question in the thread above for @PetrilloAtWork about items not previously set in the constructor.
Relatedly, Chris pointed out that directly assigning values in the header would be cleaner if we had a centralized kSignalingNaN, part of the point in Centralize definitions of NaN and -5 #24 - I have added a new header SRHelper.h for which any reviewer should feel free to comment. I will specifically tag @cjbackhouse and @PetrilloAtWork in this point.

PetrilloAtWork · 2022-02-17T00:31:11Z

Re the naming convention, I suspect there is a naming convention in StandardRecord that is different from the "common" code, in which case your additions should adhere to it (as they appear to do). I just haven't received a confirmation from @cjbackhouse yet.

Re the initialization and constructors, @FernandaPsihas and @cjbackhouse can comment. My quite strong opinion is that all data classes should move to initialisation with their declaration in the class definition, and no constructor nor destructor, unless there are specific reasons to do otherwise. While the presence of the virtual destructor has a visible impact on runtime (the data structures are one pointer larger) the change I am describing here on the construction has none, so it should not delay the merging. I still solicit a decision for the future.

cjbacchus · 2022-02-17T11:04:52Z

sbnanaobj/StandardRecord/SRHelper.h

@@ -0,0 +1,11 @@
+#ifndef SBNANAOBJ_STANDARDRECORD_SRHELPER_H


Not sure I like SRHelper as a name. Maybe SRConstants?

Done - checked only in that it builds.

cjbacchus · 2022-02-17T11:08:46Z

All looks good to me, apart from a comment about the filename.

Which fields in SRSlice are uninitialized? Anything that's an object, or a vector of anything, will default initialize properly.

I have no problem with moving all the initialization into the headers and removing most (all?) of the constructors. I think you just volunteered to do the grunt work Gianluca 😛

There is indeed a local naming convention in force. We should formalize it somewhere and debate some of its darker corners.

brucehoward-physics · 2022-02-17T16:17:30Z

@cjbackhouse

Which fields in SRSlice are uninitialized? Anything that's an object, or a vector of anything, will default initialize properly.

Yeah in SRSlice they are objects or vectors, so okay cool.

brucehoward-physics · 2022-02-17T19:07:16Z

Okay added kUninitializedInt and used it in the SRNuID.h assignments. I should say the floats could use a different other than -9999 (some could use -5 for example) for the defaults but I liked the idea of them all taking such a number. If there is push-back I can change further (especially since many slices will end up with the defaults).

brucehoward-physics added 11 commits February 11, 2022 10:33

Add variables to SRSlice for NeutrinoID features

1d6ccfa

updated classes def

f061080

Update name of one var and the capitalization in the comments.

4b80ef8

update class

a0aecfd

Break NuID feature scores into separate SR object to be written to SR…

03c117d

…Slice

update classes_def

ef2aa45

Adding SRNuID to classes_def.xml

8e9aacf

update SRNuID var names

318f105

update classes_def

f6148f5

make the unsigned int objects in SRNuID be int instead

2137b71

update checksum for SRNuID

474c851

brucehoward-physics requested review from cjbacchus, FernandaPsihas, PetrilloAtWork and henrylay97 February 11, 2022 18:36

cjbacchus approved these changes Feb 11, 2022

View reviewed changes

PetrilloAtWork requested changes Feb 16, 2022

View reviewed changes

henrylay97 approved these changes Feb 16, 2022

View reviewed changes

brucehoward-physics added 3 commits February 16, 2022 13:42

Address comments in PR 44 and partly introduce new SRHelper with kSig…

3b423d3

…nalingNaN to centralize the signaling NaN (right now just used in SRSlice and SRNuID), could be extended further.

Switch to using braces

b66ed22

remove unnecessary include that was moved to SRSlice

0b1bb48

PetrilloAtWork reviewed Feb 16, 2022

View reviewed changes

Make kSignalingNaN a constexpr

8abd7bb

PetrilloAtWork approved these changes Feb 17, 2022

View reviewed changes

cjbacchus approved these changes Feb 17, 2022

View reviewed changes

cjbacchus mentioned this pull request Feb 17, 2022

Centralize definitions of NaN and -5 #24

Open

brucehoward-physics added 2 commits February 17, 2022 10:28

Change from SRHelper to SRConstants for the centralized definitions

e8cff4f

Add kUninitializedInt

127e612

miquelnebot mentioned this pull request Feb 23, 2022

Getting (Pandora) Nu Score BDT inputs from Pandora into CAF files SBNSoftware/sbncode#229

Closed

jzennamo removed the request for review from FernandaPsihas March 16, 2022 15:49

miquelnebot changed the base branch from release/SBN2021C to release/SBN2022A March 23, 2022 12:12

miquelnebot merged commit e09fdbd into release/SBN2022A Mar 24, 2022

miquelnebot mentioned this pull request Apr 13, 2022

SBNB2022A to develop Feature/howard nuid features #61

Closed

	// \author $Author: howard@fnal.gov
	// \author howard@fnal.gov

	const float kSignalingNaN = std::numeric_limits<float>::signaling_NaN();
	constexpr float kSignalingNaN = std::numeric_limits<float>::signaling_NaN();

Feature/howard v09370103p01 nuid features #44

Feature/howard v09370103p01 nuid features #44

Conversation

brucehoward-physics commented Feb 11, 2022

PetrilloAtWork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjbacchus commented Feb 16, 2022

henrylay97 left a comment

Choose a reason for hiding this comment

brucehoward-physics commented Feb 16, 2022

PetrilloAtWork left a comment

Choose a reason for hiding this comment

PetrilloAtWork Feb 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brucehoward-physics commented Feb 16, 2022

PetrilloAtWork commented Feb 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjbacchus commented Feb 17, 2022

brucehoward-physics commented Feb 17, 2022

brucehoward-physics commented Feb 17, 2022 • edited Loading

PetrilloAtWork Feb 16, 2022 •

edited

Loading

brucehoward-physics commented Feb 17, 2022 •

edited

Loading