[WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference #24

dominikpanek · 2022-08-31T08:47:54Z

No description provided.

…cidence code

…ences and remarks from pawel

BlurredChoise

Well done, but there are a lot of places that need to be tweaked to make things work better now and in the future.

You should modify the code as in the comments provided. Correct not only new code changes but also old ones, because the current code is suboptimal, has a lot of error prone sites and is difficult to read.

Also add descriptions of the functions.

And finally, I dont see the declarations of functions in the file EventAnalysis.h

BlurredChoise · 2022-09-22T12:05:12Z

goja/EventAnalysis.cpp

@@ -232,6 +232,56 @@ EventType verify_type_of_coincidence_castor(const Hit &h1, const  Hit &h2) {
  return kUnspecified;
 }

+EventType verify_type_of_triple_coincidence(const Hit &h1, const  Hit &h2, const  Hit &h3) {


Too many if{}else{} plus this implantation is easy to cause some potential errors.

In my opinion you should replace this function with following set of functions:

bool all_same_event(const vector<Hit>& hits) { unsigned int nhits = hits.size(); if ( nhits == 0 ) { return false; } const int ref_id = hits.at(0).eventID; return std::all_of(hits.cbegin(), hits.cend(), [ref_id](Hit& hit){ return hit.eventID == ref_id; }); } bool not_phantom_compton(const vector<Hit>& hits) { return std::all_of(hits.cbegin(), hits.cend(), [](Hit& hit){ return hit.nPhantomCompton == 0; }); } bool all_crystal_compton(const vector<Hit>& hits) { return std::all_of(hits.cbegin(), hits.cend(), [](Hit& hit){ return hit.nCrystalCompton == 1; }); } EventType verify_type_of_triple_coincidence(const std::vector<Hit>& hits) { if ( all_same_event(hits) ) { //true, phantom-scattered and detector-scattered if ( not_phantom_compton(hits) ) { if ( all_crystal_compton(hits) ) { return kTrue; } return kDetectorScattered; } return kPhantomScattered; } //otherwise --> accidental return kAccidental; }

You should use these 3 new function in other part of code too.
For example, function EventType verify_type_of_coincidence(const Hit &h1, const Hit &h2)
can be replaced by EventType verify_type_of_coincidence(const vector<Hit>& hits)

BlurredChoise · 2022-09-22T12:07:58Z

goja/EventAnalysis.cpp

 //================================================================================
 // MAIN ANALYSIS FUNCTION
 //================================================================================

-void analyze_event(vector<Hit> &hits, bool hits_are_singles)
+void analyze_event(vector<Hit> &hits, bool hits_are_singles, bool triple_coincidence)


As I understand these variables are not modified by by function, hence:
void analyze_event(const vector<Hit> &hits, const bool hits_are_singles, const bool triple_coincidence)

BlurredChoise · 2022-09-22T12:09:57Z

goja/EventAnalysis.cpp

@@ -326,6 +430,13 @@ void analyze_event(vector<Hit> &hits, bool hits_are_singles)
  if (N==MAX_N and N0<=MAX_N0) print_coincidences(selected_hits);
  //if (N>=2 and N<=MAX_N and N0<=MAX_N0) print_coincidences(selected_hits);

+  if (triple_coincidences) {
+  if (N==MAX_N and N0<=MAX_N0) print_coincidences(selected_hits);


This is error fragile usage of the if().
You should use brackets: if (N==MAX_N and N0<=MAX_N0) {print_coincidences(selected_hits);}

wkrzemien · 2022-09-28T08:15:05Z

goja/EventAnalysis.cpp

@@ -399,5 +510,4 @@ void RunTests()
  assert(single4.eventID == 10); // 10 is a value set manually
  assert(single4.volumeID == 9); // 9 is a value set manually
  assert(std::abs(single4.edep - 140) < epsilon); // Energy of single == sum of energies of hits: 10+80+50 = 140
-}
-}
+}


Missing end of line

…hanges were applied in the EventAnalysis.cpp and the declarations of new functions were added in th EventAnalysis.h

BlurredChoise · 2022-10-12T13:09:28Z

goja/EventAnalysis.h

@@ -51,7 +63,9 @@ Hit merge_hits(const std::vector<Hit> &hits, const AveragingMethod winner);

  void print_coincidence(const Hit& h1, const Hit& h2);
  void print_coincidences(const std::vector<Hit>& hits);
+  void print_triple_coincidences(const std::vector<Hit>& hits, PROMPT_E_TH);


incorrectly declared method, causes compilation errors.
Please make sure that code compiles correctly before you send changes to github.

BlurredChoise · 2022-10-12T13:10:36Z

goja/EventAnalysis.h

@@ -41,8 +41,20 @@ Hit merge_hits(const std::vector<Hit> &hits, const AveragingMethod winner);
  /// returns number of singles above noise energy threshold, number of singles above Compton energy threshold, and selected singles
  std::tuple<int, int, std::vector<Hit>> select_coincident_singles(const std::vector<Hit> &hits, double compton_energy_threshold);

+  /// returns the event ID of the hit


I guess this not the description of function all_same_event(...)

BlurredChoise · 2022-10-12T13:11:11Z

goja/EventAnalysis.h

+  /// returns the event ID of the hit
+  bool all_same_event(const std::vector<Hit>& hits);
+
+  /// returns 0 if the hit was not scattered in phantom


incorrect description

Comments were corrected. As for the comments in the code they concern the functions below them.

BlurredChoise · 2022-10-12T13:11:16Z

goja/EventAnalysis.h

+  /// returns 0 if the hit was not scattered in phantom
+  bool not_phantom_compton(const std::vector<Hit>& hits);
+
+  /// returns 1 if there was not any scattering in the detector


incorrect description

it returns bool not int so a good desciption in:

Checks if none of hits is from the Compton scattering in the phantom @param: hits - list of hits from the event @return: if at least one photon scattered in the phantom it returns false, otherwise true

BlurredChoise · 2022-10-12T13:16:30Z

goja/EventAnalysis.h

  // Below function contains simple definition of coincidence that does not take into account Rayleigh scatterings
-  EventType verify_type_of_coincidence(const Hit &h1, const Hit &h2);
+  EventType verify_type_of_coincidence(const std::vector<Hit>& hits);


maybe I'm picking on, but the function name is misleading: this method returns type of event but name suggests that this function returns bool value. So maybe good idea is to replace "verify" with "get".

That is true, but these functions were already declared a long time ago and we didn't want to change it.

… Mateusz's remarks.

BlurredChoise

Please make sure this code compiles correctly ( -c++ compiler does not return errors) at this moments github shows a lot of errors which means you send a code which does not work. If you need help let me know - we can discuss how to fix some errors, but I think you can do it by yourself.

BlurredChoise · 2022-10-19T09:29:30Z

goja/EventAnalysis.h

+  /// returns 0 if the hit was not scattered in phantom
+  bool not_phantom_compton(const std::vector<Hit>& hits);
+
+  /// returns 1 if there was not any scattering in the detector


it returns bool not int so a good desciption in:

Checks if none of hits is from the Compton scattering in the phantom @param: hits - list of hits from the event @return: if at least one photon scattered in the phantom it returns false, otherwise true

BlurredChoise · 2022-10-19T09:38:32Z

goja/EventAnalysis.h

@@ -63,7 +63,7 @@ Hit merge_hits(const std::vector<Hit> &hits, const AveragingMethod winner);

  void print_coincidence(const Hit& h1, const Hit& h2);
  void print_coincidences(const std::vector<Hit>& hits);
-  void print_triple_coincidences(const std::vector<Hit>& hits, PROMPT_E_TH);
+  void print_triple_coincidences(const std::vector<Hit>& hits, double PROMPT_E_TH);


please use lowercase when you add an argument to function. Uppercase is dedicated for the GLOBAL variables.
but global variables are generally bad - you should not create global variable, instead you should create a namespace with some constant variables).

So if you want to have a set of some physical constants you should create a dedicated namespace for that i.e.:

namespace MyConstants { const int kMy_int_const = 123; const double kMy_double_const = 123.0; };

and when you want to use such constant you just call:

//...some code funnyFunction(MyConstants::kMy_double_const)

Moreover, you corrected the function declaration but you didnt corrected the usage of this function - there are missing variables.

…de compiles now.

dominikpanek added 4 commits August 31, 2022 10:46

Updated version of tripple coincidences code, accoring to Pawel remarks

c497707

New verison moved to the proper place

d2a572e

Event analysis code from the develop brnach updated with tripple coin…

0c7e27a

…cidence code

Event analysis code from develop branch, updated with tripple coincid…

d2d0f2a

…ences and remarks from pawel

wkrzemien requested a review from flukson September 5, 2022 14:10

wkrzemien requested a review from BlurredChoise September 19, 2022 14:20

BlurredChoise suggested changes Sep 22, 2022

View reviewed changes

wkrzemien reviewed Sep 28, 2022

View reviewed changes

Tripple coincidence code updated with the remarks from mateusz. The c…

21b2fce

…hanges were applied in the EventAnalysis.cpp and the declarations of new functions were added in th EventAnalysis.h

BlurredChoise suggested changes Oct 12, 2022

View reviewed changes

Functions EventAnalysis.cpp and EventAnalysis.h corrected accoring to…

a7eec1a

… Mateusz's remarks.

BlurredChoise suggested changes Oct 19, 2022

View reviewed changes

dominikpanek added 2 commits November 6, 2022 13:00

Updated version of the tripple coincidence code. Error were fixed. Co…

549e819

…de compiles now.

Updated version of tripple coincidence code

cf2b60f

wkrzemien changed the title ~~Updated version of tripple coincidences code, accoring to Pawel remarks~~ [WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference Jan 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference #24

[WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference #24

dominikpanek commented Aug 31, 2022

BlurredChoise left a comment •

edited

Loading

BlurredChoise Sep 22, 2022

BlurredChoise Sep 22, 2022

BlurredChoise Sep 22, 2022

wkrzemien Sep 28, 2022

BlurredChoise Oct 12, 2022

BlurredChoise Oct 12, 2022

dominikpanek Oct 14, 2022

BlurredChoise Oct 12, 2022

dominikpanek Oct 14, 2022

BlurredChoise Oct 12, 2022

BlurredChoise Oct 19, 2022

BlurredChoise Oct 12, 2022

dominikpanek Oct 14, 2022

BlurredChoise left a comment

BlurredChoise Oct 19, 2022

BlurredChoise Oct 19, 2022

[WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference #24

Are you sure you want to change the base?

[WIP] Updated version of tripple coincidences code, accoring to Pawel remarks - left for reference #24

Conversation

dominikpanek commented Aug 31, 2022

BlurredChoise left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlurredChoise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlurredChoise left a comment •

edited

Loading