Alpha ana #124

jgalan · 2022-01-31T10:57:00Z

This PR is created to validate alphaAna implementation.

Related PR:
rest-for-physics/rawlib#46
rest-for-physics/detectorlib#33
rest-for-physics/connectorslib#8
rest-for-physics/tracklib#13

jgalan · 2022-01-31T21:48:45Z

PandaX-III pipeline is failing. Perhaps due to the new TRestRawZeroSuppresionProcess? @nkx @juanangp

https://gitlab.cern.ch/rest-for-physics/framework/-/jobs/19174479

juanangp · 2022-02-01T08:47:55Z

PandaX-III pipeline is failing. Perhaps due to the new TRestRawZeroSuppresionProcess? @nkx @juanangp

https://gitlab.cern.ch/rest-for-physics/framework/-/jobs/19174479

Indeed, looks like the issue is that we have to add a TRestRawToDetectorSignalProcess after TRestRawZeroSuppresionProcess.

@jgalan are you going to fix it in panda pipeline?

juanangp · 2022-02-01T08:54:37Z

Another issue is that we are loosing precision because former TRestRawZeroSuppresionProcess was saving data as Double_t, while now the data is stored as a Short_t in TRestRawSignal.

juanangp · 2022-02-01T16:11:43Z

Another issue is that we are loosing precision because former TRestRawZeroSuppresionProcess was saving data as Double_t, while now the data is stored as a Short_t in TRestRawSignal.

Any idea in how to fix this issue, shall we go back to the previous implementation?

jgalan · 2022-02-02T10:19:38Z

Another issue is that we are loosing precision because former TRestRawZeroSuppresionProcess was saving data as Double_t, while now the data is stored as a Short_t in TRestRawSignal.

Aham, well the data in origin is in short format. So why the result is different? Perhaps at ZeroSuppression there is an additional manipulation before being transformed to Double_t by TRestRawToDetectorSignalProcess?

jgalan · 2022-02-02T10:25:47Z

At TRestRawZeroSuppression the baseline is being subtracted when invoking GetData.

        std::vector<Int_t> pOver = s->GetPointsOverThreshold();
        for (int n = 0; n < pOver.size(); n++) {
            int j = pOver[n];
            sgn.IncreaseBinBy(j, s->GetData(j));
        }

We should avoid to remove the baseline as soon as we stay at TRestRaw domain. And we should remove it once we transform to TRestDetectorSignal.

So, using TRestRawSignal::GetRawData instead of TRestRawSignal::GetData. The problem is that when zero-ing we loss the baseline information.

The idea behind is that we always calculate the baseline so that we do not store the values inside TRestRawSignal.

An option would be to include those data members Double_t fBaseLine = 0 and Double_t fBaseLineSigma = 0 inside TRestRawSignal, so that it can be recovered even after TRestRawZeroSuppressionProcess.

Option 1: We introduce those data members at TRestRawSignal and we move REST version to 2.4.0.

Option 2: We implement zeroSuppression as a method/option at TRestRawToDetectorSignalProcess.

@nkx what do you think?

juanangp · 2022-02-02T11:20:00Z

Option 1: We introduce those data members at TRestRawSignal and we move REST version to 2.4.0.

Option 2: We implement zeroSuppression as a method/option at TRestRawToDetectorSignalProcess.

@nkx what do you think?

I would suggest to move to option 2

juanangp · 2022-02-02T16:04:51Z

I really don't understand the following observabales in TRestRawZeroSuppresionProcess:

    SetObservableValue("flattail_map", flattailmap);
    SetObservableValue("FlatTails", totoalflatN);
    SetObservableValue("NFlatTailSignals", (int)flattailmap.size());

Particularly the definition:

int Nbefore;
        if (fNPointsFlatThreshold != 512) {
            s->InitializePointsOverThreshold(TVector2(fPointThreshold, fSignalThreshold),
                                             fNPointsOverThreshold, 512);
            Nbefore = s->GetPointsOverThreshold().size();
        }
        s->InitializePointsOverThreshold(TVector2(fPointThreshold, fSignalThreshold), fNPointsOverThreshold,
                                         fNPointsFlatThreshold);

        int Nafter = s->GetPointsOverThreshold().size();
        // cout << fRawSignalEvent->GetID() << " " << s->GetID() << " " << Nbefore << " " << Nafter << endl;
        if (Nafter != Nbefore) {
            flattailmap[s->GetID()] = Nbefore - Nafter;
            totoalflatN += Nbefore - Nafter;
        }

Note that Nbefore is not initialiazed while fNPointsFlatThreshold==512, which I think should be most of the cases, it doesn't make sense to me...

Shall we keep these observables or it is a good time to get rid of them?

juanangp · 2022-02-02T18:44:20Z

Pipeline is failing because of c++ structure bindings. So looks like pipeline is still using C++14, indeed root version is still 6.10/08 in which only C++14 was supported.

@jgalan Can we upgrade it to ROOT 6.24 and compile in C++17?

jgalan · 2022-02-02T18:54:10Z

In principle I have no problem. I believe the reason behind is to keep compatibility with older systems, so installation is possible on those systems @nkx? I guess we just need someone preparing a docker image.

Perhaps, we could say, v2.3.10 is compatible with C++14, v2.3.11 will not be compatible anymore?

We have also the LFNA GitLab instance at Unizar, where we were previously executing jobs without docker and we had much more control.

If it is not possible to keep the code compatible with both?

juanangp · 2022-02-02T19:02:44Z

If it is not possible to keep the code compatible with both?

Yes, it is possible but I think we should implement C++17 features in the code, particularly structure bindings makes the coding easier.

I tested locally with C++17 and ROOT 6.24 and it is running fine to me.

jgalan · 2022-02-02T19:07:10Z

I am not a big fan of structures. If it is as relevant as to create a structure, perhaps a full class is a better option.

jgalan · 2022-02-02T19:09:21Z

I am looking at the advantages at this example https://www.geeksforgeeks.org/structured-binding-c/

And I dont really understand the benefit of linking to x_coord, would not be easier just to use p.x?

C++98 looked simple enough, why it became complex later, and then they had to redo it later again?

juanangp · 2022-02-02T20:01:43Z

Perhaps you are looking to the wrong example, structure bindings are particularly useful for e.g. std::map, for instance the part of the code that I wrote:
C++14

std::map<int,std::pair<int,double> > windowMap = makeMap();
  for(auto it = windowMap.begin(); it!=windowMap.end();it++){
              Double_t time = it->first * fIntWindow + fIntWindow/2.;
              Double_t energy = it->second.second/it->second.first;
            }

C++17

 std::map<int,std::pair<int,double> > windowMap = makeMap();
  for(const auto &[index, pair] : windowMap){
              Double_t time = index * fIntWindow + fIntWindow/2.;
              Double_t energy = pair.second/pair.first;
            }

As you can see the implementation in C++14 is not readable and error prone while in C++17 it is easy to understand.

On the other hand C++17 has many other advantages apart of structure bindings:
https://www.codingame.com/playgrounds/2205/7-features-of-c17-that-will-simplify-your-code/introduction

Still don't understand why you are against moving forward to C++17, latest version is C++20 and C++23 is on-going...

jgalan · 2022-02-03T03:14:26Z

In principle I have no problem.

Again, as I said before, I am not against.

lobis · 2022-02-03T09:37:23Z

In principle I have no problem. I believe the reason behind is to keep compatibility with older systems, so installation is possible on those systems @nkx? I guess we just need someone preparing a docker image.

Perhaps, we could say, v2.3.10 is compatible with C++14, v2.3.11 will not be compatible anymore?

We have also the LFNA GitLab instance at Unizar, where we were previously executing jobs without docker and we had much more control.

If it is not possible to keep the code compatible with both?

Hello,

I use this DockerHub image to do all my tests: https://hub.docker.com/r/lobis/root-geant4-garfieldpp/tags we can use this in the pipeline if you want. There are a good combination of ROOT / Geant4 / C++ versions available, I can make more if you want.

nkx111 · 2022-02-04T13:50:40Z

At TRestRawZeroSuppression the baseline is being subtracted when invoking GetData.
        std::vector<Int_t> pOver = s->GetPointsOverThreshold();
        for (int n = 0; n < pOver.size(); n++) {
            int j = pOver[n];
            sgn.IncreaseBinBy(j, s->GetData(j));
        }      
We should avoid to remove the baseline as soon as we stay at TRestRaw domain. And we should remove it once we transform to TRestDetectorSignal.

So, using TRestRawSignal::GetRawData instead of TRestRawSignal::GetData. The problem is that when zero-ing we loss the baseline information.

The idea behind is that we always calculate the baseline so that we do not store the values inside TRestRawSignal.

An option would be to include those data members Double_t fBaseLine = 0 and Double_t fBaseLineSigma = 0 inside TRestRawSignal, so that it can be recovered even after TRestRawZeroSuppressionProcess.

Option 1: We introduce those data members at TRestRawSignal and we move REST version to 2.4.0.

Option 2: We implement zeroSuppression as a method/option at TRestRawToDetectorSignalProcess.

@nkx what do you think?

I also stands for the option 2. It is too quick to move to 2.4.0 now.

nkx111 · 2022-02-04T13:55:39Z

I really don't understand the following observabales in TRestRawZeroSuppresionProcess:

    SetObservableValue("flattail_map", flattailmap);
    SetObservableValue("FlatTails", totoalflatN);
    SetObservableValue("NFlatTailSignals", (int)flattailmap.size());

Particularly the definition:

int Nbefore;
        if (fNPointsFlatThreshold != 512) {
            s->InitializePointsOverThreshold(TVector2(fPointThreshold, fSignalThreshold),
                                             fNPointsOverThreshold, 512);
            Nbefore = s->GetPointsOverThreshold().size();
        }
        s->InitializePointsOverThreshold(TVector2(fPointThreshold, fSignalThreshold), fNPointsOverThreshold,
                                         fNPointsFlatThreshold);

        int Nafter = s->GetPointsOverThreshold().size();
        // cout << fRawSignalEvent->GetID() << " " << s->GetID() << " " << Nbefore << " " << Nafter << endl;
        if (Nafter != Nbefore) {
            flattailmap[s->GetID()] = Nbefore - Nafter;
            totoalflatN += Nbefore - Nafter;
        }

Note that Nbefore is not initialiazed while fNPointsFlatThreshold==512, which I think should be most of the cases, it doesn't make sense to me...

Shall we keep these observables or it is a good time to get rid of them?

These lines are some rough handling to AGET abnormal tailing signals, developed very long time ago. I think they are already out-dated for 2.3.x. We can just remove them. In future we may develop specific processes to handle those signals.

fNPointsFlatThreshold=512 means to turn off that functionality in TRestRawZeroSuppresionProcess. Here 512 is also a specialized number only for AGET electronics, which is not good.

juanangp · 2022-02-04T16:00:31Z

I also stands for the option 2. It is too quick to move to 2.4.0 now.

I have already implemented option 2

jgalan · 2022-04-05T08:11:29Z

Yes, but I cannot approve it myself since I created. It seems fine to me to merge it.

jgalan · 2022-04-05T08:11:59Z

rest-for-physics/rawlib#46 pipeline is failing

juanangp · 2022-04-05T08:16:05Z

rest-for-physics/rawlib#46 pipeline is failing

This is because the docker image is not the right one, I think should be fine after merging. Similar issues have been spotted in other PR.

juanangp · 2022-04-05T08:22:21Z

rest-for-physics/rawlib#46 pipeline is failing

This is because the docker image is not the right one, I think should be fine after merging. Similar issues have been spotted in other PR.

In fact this is fixed in rest-for-physics/rawlib#51 which is not yet merged in master.

jgalan · 2022-04-05T08:25:10Z

rest-for-physics/rawlib#46 pipeline is failing

This is because the docker image is not the right one, I think should be fine after merging. Similar issues have been spotted in other PR.

In fact this is fixed in rest-for-physics/rawlib#51 which is not yet merged in master.

But rest-for-physics/rawlib#51 is also failing

jgalan · 2022-04-05T08:29:03Z

Actually rawlib at master is also failing after merging rest-for-physics/rawlib#50. Perhaps we just need to update the docker image? @lobis

juanangp · 2022-04-05T08:37:29Z

rest-for-physics/rawlib#46 pipeline is failing

This is because the docker image is not the right one, I think should be fine after merging. Similar issues have been spotted in other PR.

In fact this is fixed in rest-for-physics/rawlib#51 which is not yet merged in master.

But rest-for-physics/rawlib#51 is also failing

Indeed, this is another un-related issue to this PR after the migration to C++17. This issue arises from performing on-the-fly compilation of STL containers and hardcoding compilation flags in the code:

framework/source/framework/tools/src/TRestReflector.cxx

Line 230 in 1d60c10

system(Form("gcc %s -std=c++11 -I`root-config --incdir` "

A patch could be

system(Form("gcc %s `root-config --cflags` "

@lobis we had a chat about this, we don't have any related issue?

jgalan · 2022-04-05T09:02:00Z

However, the rawlib pipeline seems to fail at the cmake level.

-- Testing disabled (Disabled by default, enabled via -DTEST=ON flag)
-- Check for ROOT_DICT_OUTPUT_DIR: /builds/rest-for-physics/rawlib/framework/build/rootdict
-- Check for ROOT_DICT_CINT_DEFINITIONS: 
-- External dependencies:
CMake Error at source/framework/external/CMakeLists.txt:4 (include):
  include could not find load file:

    FetchContent


-- making build files for /builds/rest-for-physics/rawlib/framework/source/framework, schema evolution: 
specified sub-dirs: external/tinyxml;tools;core;analysis
Empty submodule dir: libraries/track. Option: RESTLIB_TRACK=OFF
Empty submodule dir: libraries/geant4. Option: RESTLIB_GEANT4=OFF

https://gitlab.cern.ch/rest-for-physics/rawlib/-/jobs/20856420

juanangp · 2022-04-05T09:06:13Z

However, the rawlib pipeline seems to fail at the cmake level.

master is failing because we don't have the proper docker image.

Docker image is updated on rest-for-physics/rawlib#51, however is failing becasue of

framework/source/framework/tools/src/TRestReflector.cxx

Line 230 in 1d60c10

system(Form("gcc %s -std=c++11 -I`root-config --incdir` "

jgalan · 2022-04-05T09:11:34Z

So, then why dont try to update it at that PR using system(Form("gcc %s root-config --cflags " and see if it goes through? I guess it makes sense, so that we do not force c++11.

…lphaAna

lobis · 2022-03-19T22:11:53Z

cmake/FindGarfieldOld.cmake


 #message(STATUS Garfield_INCLUDE_DIRS ${Garfield_INCLUDE_DIRS})

 find_library(Garfield_LIBRARIES NAMES libGarfield.so Garfield
        HINTS ${Garfield_DIR}/lib ${Garfield_LIB_DIR}
-        $ENV{GARFIELD_HOME}/lib)
+        $ENV{GARFIELD_DIR}/lib)


https://cmake.org/cmake/help/latest/command/find_library.html
If I understand this correctly, this change should be equivalent to removing $ENV{GARFIELD_HOME}/lib) from the macro. Why is this needed?

I believe this line is not needed and it was actually removed, I don't understand how it end up in this PR.

As fas as I remember some commits were cherry-pick from this PR to master. Perhaps something when wrong in between.

This branch is behind master, perhaps it will be fixed once master is merged in this branch. Highly recommended.

I guess thats why "outdated" appears next to the file name

…lphaAna

jgalan added 9 commits January 31, 2022 11:44

Adding 01.alphaTrack validation routine

db03914

Pipeline/trexdm Removing forking feature from pipeline

dd4e374

Fixing pipeline

18bd869

examples/01.alphaTrack adapting to new ZeroSuppresion scheme

e9b5649

Updating 01.alphaTrack Validate.C

4f1239d

examples/01.alphaTrack. Minor README update

e6e34c0

examples/01.alphaTrack increasing validation range

dc1ba5b

pipeline/trexdm fixing chain connectivity

b10865c

pipeline/trexdm fixing pipeline

bbe7181

juanangp added 2 commits February 2, 2022 19:15

Trying do address trex pipeline

03f8328

Updating alphaTrack example with current code changes

96d7bc5

juanangp and others added 2 commits April 5, 2022 13:38

Merge branch 'master' into alphaAna

15e69c2

Merge branch 'master' of github.com:rest-for-physics/framework into a…

3c031f9

…lphaAna

juanangp requested a review from lobis April 18, 2022 10:57

lobis previously approved these changes Apr 20, 2022

View reviewed changes

jgalan and others added 2 commits April 20, 2022 21:47

Merge branch 'master' into alphaAna

46d6a36

Fixing compilation error after merge with master

ff37137

jgalan dismissed stale reviews from lobis and juanangp via ff37137 April 22, 2022 09:03

juanangp previously approved these changes Apr 25, 2022

View reviewed changes

Increasing tracklib submodule to 1.3

ccd048a

jgalan dismissed juanangp’s stale review via ccd048a April 25, 2022 11:12

juanangp approved these changes Apr 25, 2022

View reviewed changes

juanangp requested a review from lobis April 25, 2022 11:21

juanangp added 4 commits April 25, 2022 13:37

Increasing rawlib submodule to 1.6

e138c59

Increasing connectorslib submodule to 1.2

5e0610f

Increasing detectorlib submodule to 1.7

8a33774

Updated TRestVersion.h to v2.3.12

792df26

lobis approved these changes Apr 25, 2022

View reviewed changes

Merge branch 'master' of github.com:rest-for-physics/framework into a…

0cf8f3c

…lphaAna

juanangp merged commit 0153c04 into master Apr 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alpha ana #124

Alpha ana #124

jgalan commented Jan 31, 2022 •

edited by pull-request-badge bot

Loading

jgalan commented Jan 31, 2022

juanangp commented Feb 1, 2022

juanangp commented Feb 1, 2022

juanangp commented Feb 1, 2022

jgalan commented Feb 2, 2022

jgalan commented Feb 2, 2022 •

edited

Loading

juanangp commented Feb 2, 2022

juanangp commented Feb 2, 2022

juanangp commented Feb 2, 2022

jgalan commented Feb 2, 2022 •

edited

Loading

juanangp commented Feb 2, 2022

jgalan commented Feb 2, 2022

jgalan commented Feb 2, 2022 •

edited

Loading

juanangp commented Feb 2, 2022

jgalan commented Feb 3, 2022

lobis commented Feb 3, 2022

nkx111 commented Feb 4, 2022

nkx111 commented Feb 4, 2022

juanangp commented Feb 4, 2022 •

edited

Loading

jgalan commented Apr 5, 2022

jgalan commented Apr 5, 2022

juanangp commented Apr 5, 2022

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022

jgalan commented Apr 5, 2022

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022 •

edited

Loading

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022

lobis Mar 19, 2022

juanangp Apr 20, 2022

jgalan Apr 20, 2022

jgalan Apr 20, 2022

Alpha ana #124

Alpha ana #124

Conversation

jgalan commented Jan 31, 2022 • edited by pull-request-badge bot Loading

jgalan commented Jan 31, 2022

juanangp commented Feb 1, 2022

juanangp commented Feb 1, 2022

juanangp commented Feb 1, 2022

jgalan commented Feb 2, 2022

jgalan commented Feb 2, 2022 • edited Loading

juanangp commented Feb 2, 2022

juanangp commented Feb 2, 2022

juanangp commented Feb 2, 2022

jgalan commented Feb 2, 2022 • edited Loading

juanangp commented Feb 2, 2022

jgalan commented Feb 2, 2022

jgalan commented Feb 2, 2022 • edited Loading

juanangp commented Feb 2, 2022

jgalan commented Feb 3, 2022

lobis commented Feb 3, 2022

nkx111 commented Feb 4, 2022

nkx111 commented Feb 4, 2022

juanangp commented Feb 4, 2022 • edited Loading

jgalan commented Apr 5, 2022

jgalan commented Apr 5, 2022

juanangp commented Apr 5, 2022

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022

jgalan commented Apr 5, 2022

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022 • edited Loading

juanangp commented Apr 5, 2022

jgalan commented Apr 5, 2022

lobis Mar 19, 2022

Choose a reason for hiding this comment

juanangp Apr 20, 2022

Choose a reason for hiding this comment

jgalan Apr 20, 2022

Choose a reason for hiding this comment

jgalan Apr 20, 2022

Choose a reason for hiding this comment

jgalan commented Jan 31, 2022 •

edited by pull-request-badge bot

Loading

jgalan commented Feb 2, 2022 •

edited

Loading

jgalan commented Feb 2, 2022 •

edited

Loading

jgalan commented Feb 2, 2022 •

edited

Loading

juanangp commented Feb 4, 2022 •

edited

Loading

jgalan commented Apr 5, 2022 •

edited

Loading