Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

more processes and metadata #1

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

more processes and metadata #1

wants to merge 4 commits into from

Conversation

nkx111
Copy link
Member

@nkx111 nkx111 commented Jun 15, 2022

added v2.2 readout and v2.2 process: TRestSignalZeroSuppresionProcess
use preprocessor to define legacy processes members, instead of inheriting TRestLegacyProcess. This prevents a warning:

Warning in <TStreamerInfo::CompareContent>: One base class of the on-file layout version 4 and of the in memory layout version 4 for 'TRestSignalZeroSuppresionProcess' is different: 'TRestEventProcess' vs 'TRestLegacyProcess'

@nkx111 nkx111 marked this pull request as ready for review June 26, 2022 10:30
@nkx111 nkx111 requested a review from juanangp June 29, 2022 01:00
@@ -1,5 +1,7 @@
set(LibraryVersion "1.0")
add_definitions(-DLIBRARY_VERSION="${LibraryVersion}")

COMPILELIB("")
set(deps detector)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is conceptually wrong to add dependencies to any legacy class or library

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably we just need the data members and the name of the class being preserved for data being read from previous ROOT files.


#include "TRestDetectorDriftVolume.h"

typedef TRestDetectorDriftVolume TRestDriftVolume;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class has been renamed? Not quite understand this class


#include "TRestDetectorGainMap.h"

typedef TRestDetectorGainMap TRestGainMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here? The typedef shouldn't be included in TRestDetectorGainMap.h?


#include "TRestDetectorGas.h"

typedef TRestDetectorGas TRestGas;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same


//! An event data type that register a vector of TRestHits,
//! allowing us to save a 3-coordinate position and energy.
class TRestHitsEvent : public TRestEvent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that TRestHitsEvent should inherit from TRestLegacyEvent with some protection added

@@ -52,4 +52,23 @@ class TRestLegacyProcess : public TRestEventProcess {

ClassDefOverride(TRestLegacyProcess, 0);
};

#define LegacyProcessDef(thisname, newname, n) \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not fan of macros and I don't understand the need of a macro here

@@ -26,7 +26,7 @@
#include "TRestLegacyProcess.h"

//! A process to identify signal and remove baseline noise from a TRestRawSignalEvent.
class TRestRawZeroSuppresionProcess : public TRestLegacyProcess {
class TRestRawZeroSuppresionProcess : public TRestEventProcess {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRestRawZeroSuppresionProcess should inherit from TRestLegacyProcess

#include <TString.h>
#include <TVector2.h>

class TRestSignal : public TObject {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRestSignal seems quite obsolete, what is the need of implemented inside legacylib?

#include "TRestEvent.h"
#include "TRestSignal.h"

class TRestSignalEvent : public TRestEvent {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here TRestSignalEvent looks extremelly old, do we have a real need to keep in legacylib?

Also, I think that any legacy class shouldn't implement any data processing, it is just for compatibility with old root files.

#include "TRestLegacyProcess.h"

//! A process to identify signal and remove baseline noise from a TRestRawSignalEvent.
class TRestSignalZeroSuppresionProcess : public TRestEventProcess {
Copy link
Member

@juanangp juanangp Jun 29, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TRestSignalZeroSuppresionProcess looks quite similar to TRestRawZeroSuppresionProcess a typedef cannot be used here?

@jgalan
Copy link
Member

jgalan commented Jul 1, 2022

I dont understand, TRestDetectorXYZ classes are becoming obsolete?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants