Feature tidy up #189

EdAtkin · 2024-10-28T14:56:36Z

Pull request description

Mainly tidying up some bits of code especially the FarDetectorStruct class. Removing allocation of lots of arrays on the heap to use std::vectors instead. Also a general bit of tidy up here and there.

Changes or fixes

Rename of FDMC struct to FarDetectorStruct. Also moving many of the members of this struct to be vectors. Small clean up of splineFDBase class.

Examples

e.g. int* modes = new int[nEvents]; -> std::vector modes; modes.resize(nEvents);

KSkwarczynski · 2024-10-28T15:56:09Z

samplePDF/FarDetectorCoreInfoStruct.h

+
+  bool *isNC;
+
+  // histo pdf bins


Why is it raw pointer while everyhitng else is vector?

Read about std::vector bool, it doesn't behave like other std::vector objects, or just try printing the address of the first three entries of auto myvecbool = std::vector bool {true,true,false};

I feel dumb for not knowing this...

In this case there should be a destructor?

Probably there should be a default constructor:

FarDetectorCoreInfo() : isNC{nullptr} {} ~FarDetectorCoreInfo(){delete [] isNC;}

luketpickering · 2024-10-29T08:50:24Z

samplePDF/CMakeLists.txt

@@ -2,7 +2,7 @@ set(HEADERS
    samplePDFBase.h
    samplePDFFDBase.h
    Structs.h
-    FDMCStruct.h
+	FarDetectorCoreInfoStruct.h


I know its minor, but I'd remove struct from the file name, its very common to have a file named after the class it is defining. And having 'struct' in the name of a class/struct is just noise. (I also think 'Core' is noise here, and FarDetData or something is a cleaner name, but your name is absolutely fine and we don't need to argue forever about names)

luketpickering · 2024-10-29T08:51:53Z

samplePDF/samplePDFFDBase.cpp

@@ -785,7 +782,7 @@ void samplePDFFDBase::SetupNormParameters(){
 //A way to check whether a normalisation parameter applies to an event or not
 void samplePDFFDBase::CalcXsecNormsBins(int iSample){

-  fdmc_base *fdobj = &MCSamples[iSample];
+  FarDetectorCoreInfo *fdobj = &MCSamples[iSample];


Its up to you, it really is minor, but weaning people off using pointers when they don't need to, you might consider:

FarDetectorCoreInfo &fdobj = MCSamples[iSample]; //... fdobj.someprop = something;

but it depends on how people feel about reference/pointer/copy semantics. You do have to be careful and confident... maybe taking pointers is clearer that it isn't a copy, as if someone misses that ampersand then it doesn't do what you expect. (you could delete the copy/move constructors of FarDetectorCoreInfo to elide this

struct FarDetectorCoreInfo{ FarDetectorCoreInfo(FarDetectorCoreInfo const &other) = delete; FarDetectorCoreInfo(FarDetectorCoreInfo &&other) = delete; FarDetectorCoreInfo& operator=(FarDetectorCoreInfo const &other) = delete; FarDetectorCoreInfo& operator=(FarDetectorCoreInfo &&other) = delete; }

I'd maybe do this anyway and see if you do copy/move anywhere. If you always construct in place with emplace_back or resize, then I think its fine/best to not have copy constructors/assignment

luketpickering · 2024-10-29T08:54:16Z

samplePDF/samplePDFFDBase.cpp

@@ -785,7 +782,7 @@ void samplePDFFDBase::SetupNormParameters(){
 //A way to check whether a normalisation parameter applies to an event or not
 void samplePDFFDBase::CalcXsecNormsBins(int iSample){

-  fdmc_base *fdobj = &MCSamples[iSample];
+  FarDetectorCoreInfo *fdobj = &MCSamples[iSample];

  for(int iEvent=0; iEvent < fdobj->nEvents; ++iEvent){
    std::list< int > XsecBins = {};


I really don't see why we use a list here, should be a vector

luketpickering · 2024-10-29T08:56:51Z

samplePDF/samplePDFFDBase.cpp

+  fdobj->mode.resize(nEvents, &fdobj->Unity_Int);
+  fdobj->nxsec_norm_pointers.resize(nEvents);
+  fdobj->xsec_norm_pointers.resize(nEvents);
+  fdobj->xsec_norms_bins = new std::list< int >[nEvents];


I don't think we should manually manage the memory of an STL container designed to obviate manual memory management. Also it should just be a vector

Oh, this might actually be a find/replace issue, I don't even think the syntax is correct.

luketpickering · 2024-10-29T08:58:11Z

samplePDF/samplePDFFDBase.cpp

+  fdobj->total_weight_pointers.resize(nEvents);
+  fdobj->Target.resize(nEvents, 0);
+#ifdef _LOW_MEMORY_STRUCTS_
+  fdobj->osc_w_pointer.resize(nEvents, &fdobj->Unity_F);


we should just have a single fdobj->Unity data member with the type changing based upon this define. My 'tidy up' PR will make this trivial, so don't need to change yet, just a comment for future.

luketpickering · 2024-10-29T08:58:52Z

samplePDF/samplePDFFDBase.cpp


-    fdobj->x_var[iEvent] = &fdobj->Unity;
-    fdobj->y_var[iEvent] = &fdobj->Unity;
+  for(int iEvent = 0 ; iEvent < fdobj->nEvents ; ++iEvent){


std::fill_n(fdobj->isNC, fdobj->nEvents, false);

…g anyway as ROOT does not return these as -999. Also a bit of cleaning up of the knot and coefficient setting

…resize

- xsec_norm_bins is now a vector - delete the copy constructor/assignment and move assignment, add default constructor

EdAtkin requested a review from luketpickering October 28, 2024 14:56

github-actions bot added Nu Osc/Xsec Related with neutrino interactions or oscialtions Samples labels Oct 28, 2024

KSkwarczynski reviewed Oct 28, 2024

View reviewed changes

luketpickering approved these changes Oct 29, 2024

View reviewed changes

luketpickering force-pushed the feature_TidyUp branch from a695a6b to bc81ac9 Compare October 31, 2024 11:41

EdAtkin and others added 11 commits October 31, 2024 12:47

removing checks on magic numbers i.e. -999 which aren't doing anythin…

79a6e31

…g anyway as ROOT does not return these as -999. Also a bit of cleaning up of the knot and coefficient setting

renaming splineFile to be SplineHandler which I hope is clearer

7ed33cf

renaming the file containing FarDetectorCoreInformation as well

c6325d2

removing as many arrays as possible and using vectors

c00da94

changing the default setup of the values for the vectors to just use …

3ad1620

…resize

minor change because of rename of core struct

7fc2f01

a small clean up of the spline loading

ca50707

Changing the Check on Yaml nodes if they exist

5ca59ff

remove list, make FarDetectorCoreInfo less easy to misuse by value

c34cac5

- xsec_norm_bins is now a vector - delete the copy constructor/assignment and move assignment, add default constructor

update to new covariance interface with parameter type enum

74325ec

remove pointless cast

ef6032a

luketpickering force-pushed the feature_TidyUp branch from bc81ac9 to ef6032a Compare October 31, 2024 13:14

remove unnecessary include that referenced old header in python bindings

a812e4b

luketpickering marked this pull request as ready for review October 31, 2024 13:37

luketpickering requested review from henry-wallace-phys, ewanwm and dbarrow257 as code owners October 31, 2024 13:37

luketpickering merged commit 8846589 into develop Oct 31, 2024
11 checks passed

luketpickering deleted the feature_TidyUp branch October 31, 2024 13:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature tidy up #189

Feature tidy up #189

EdAtkin commented Oct 28, 2024

KSkwarczynski Oct 28, 2024

luketpickering Oct 28, 2024 •

edited

Loading

KSkwarczynski Oct 28, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

luketpickering Oct 29, 2024

Feature tidy up #189

Feature tidy up #189

Conversation

EdAtkin commented Oct 28, 2024

Pull request description

Changes or fixes

Examples

Choose a reason for hiding this comment

luketpickering Oct 28, 2024 • 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

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

luketpickering Oct 28, 2024 •

edited

Loading