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.

Sign up for GitHub

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

Adding function to read input parameters from the RML, and some renaming #72

Closed
wants to merge 3 commits into from
Closed

Adding function to read input parameters from the RML, and some renaming #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3f397b0
Adding function to read input parameters from the RML, and some renaming
cmargalejo Jul 25, 2022
f462514
Removing default values from the header
cmargalejo Jul 25, 2022
7c657ae
Fix typo
cmargalejo Jul 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions inc/TRestRawSignalChannelActivityProcess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,34 +45,36 @@ class TRestRawSignalChannelActivityProcess : public TRestEventProcess {
TRestDetectorReadout* fReadout; //!
#endif

void InitFromConfigFile() override;

void Initialize() override;

void LoadDefaultConfig();

protected:
/// The value of the lower signal threshold to add it to the histogram
Double_t fLowThreshold = 25;
Double_t fLowThreshold;

/// The value of the higher signal threshold to add it to the histogram
Double_t fHighThreshold = 50;
Double_t fHighThreshold;

/// The number of bins at the daq channels histogram
Int_t fDaqChannels = 300;
Int_t fDaqHistogramChannels;
Copy link
Member

Choose a reason for hiding this comment

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

Why to remove the default values from header? Providing the default value on the headers is now the stardard way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed them because I had defined them in the .cxx file, to keep coherence with TRestDetectorSignalChannelActivityProcess. But I am going to put them back here if it is the standard way now. Should I then also change the cxx, so that the default values are not defined in two places?

Copy link
Member

@jgalan jgalan Jul 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You should completely remove InitFromConfigFile implementation. No need to implement GetParameter for basic parameters, in case it is necessary, you should make a call to TRestEventProcess::InitFromConfigFile or TRestMetadata::InitFromConfigFile.

See for example the following implementation of TRestAxionOptics::InitFromConfigFile.

https://github.com/rest-for-physics/axionlib/blob/master/src/TRestAxionOptics.cxx

The line TRestMetadata::InitFromConfigFile on the previous file will be responsible to read all the parameters, following the pattern fVarName to varName and it will initialise it with the default value if not given in the RML.

In your case I believe you dont need to implement InitFromConfigFile. If it is not implemented, then the default is that an instance will call TRestEventProcess::InitFromConfigFile method.


/// The number of bins at the readout channels histogram
Int_t fReadoutChannels = 128;
Copy link
Member

Choose a reason for hiding this comment

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

It is fine to rename the metadata members althought I would avoid to do that frequently.

But, in order to apply proper class versioning, i.e. ROOT schema evolution, it is required to increase the ClassDefOverride number to 4.

I.e. the last line in this file

 ClassDefOverride(TRestRawSignalChannelActivityProcess, 3);

should be updated to

 ClassDefOverride(TRestRawSignalChannelActivityProcess, 4);

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think it is better to put the original names again. They are still appropriate names and we would avoid any issues with past versions. Do you agree?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree with both choices, still changing names should be not be a big deal as soon as the class version is increased.

Int_t fReadoutHistogramChannels;

/// The first channel at the daq channels histogram
Int_t fDaqStartChannel = 4320;
Int_t fDaqStartChannel;

/// The last channel at the daq channels histogram
Int_t fDaqEndChannel = 4620;
Int_t fDaqEndChannel;

/// The first channel at the readout channels histogram
Int_t fReadoutStartChannel = 0;
Int_t fReadoutStartChannel;

/// The last channel at the readout channels histogram
Int_t fReadoutEndChannel = 128;
Int_t fReadoutEndChannel;

/// The daq channels histogram
TH1D* fDaqChannelsHisto; //!
Expand Down Expand Up @@ -121,12 +123,12 @@ class TRestRawSignalChannelActivityProcess : public TRestEventProcess {
RESTMetadata << "Low signal threshold activity : " << fLowThreshold << RESTendl;
RESTMetadata << "High signal threshold activity : " << fHighThreshold << RESTendl;

RESTMetadata << "Number of daq histogram channels : " << fDaqChannels << RESTendl;
RESTMetadata << "Number of daq histogram channels : " << fDaqHistogramChannels << RESTendl;
RESTMetadata << "Start daq channel : " << fDaqStartChannel << RESTendl;
RESTMetadata << "End daq channel : " << fDaqEndChannel << RESTendl;

#ifdef REST_DetectorLib
RESTMetadata << "Number of readout histogram channels : " << fReadoutChannels << RESTendl;
RESTMetadata << "Number of readout histogram channels : " << fReadoutHistogramChannels << RESTendl;
RESTMetadata << "Start readout channel : " << fReadoutStartChannel << RESTendl;
RESTMetadata << "End readout channel : " << fReadoutEndChannel << RESTendl;
#else
Expand Down
48 changes: 34 additions & 14 deletions src/TRestRawSignalChannelActivityProcess.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,40 +175,43 @@ void TRestRawSignalChannelActivityProcess::InitProcess() {
#ifdef REST_DetectorLib
fReadout = GetMetadata<TRestDetectorReadout>();

RESTDebug << "TRestRawSignalChannelActivityProcess::InitProcess. Readout pointer : " << fReadout << RESTendl;
if (GetVerboseLevel() >= TRestStringOutput::REST_Verbose_Level::REST_Info && fReadout) fReadout->PrintMetadata();
RESTDebug << "TRestRawSignalChannelActivityProcess::InitProcess. Readout pointer : " << fReadout
<< RESTendl;
if (GetVerboseLevel() >= TRestStringOutput::REST_Verbose_Level::REST_Info && fReadout)
fReadout->PrintMetadata();
#endif

if (!fReadOnly) {
fDaqChannelsHisto = new TH1D("daqChannelActivityRaw", "daqChannelActivityRaw", fDaqChannels,
fDaqChannelsHisto = new TH1D("daqChannelActivityRaw", "daqChannelActivityRaw", fDaqHistogramChannels,
fDaqStartChannel, fDaqEndChannel);
#ifdef REST_DetectorLib
if (fReadout) {
fReadoutChannelsHisto = new TH1D("rChannelActivityRaw", "readoutChannelActivity",
fReadoutChannels, fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto =
new TH1D("rChannelActivityRaw", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_OneSignal =
new TH1D("rChannelActivityRaw_1", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_1", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_OneSignal_High =
new TH1D("rChannelActivityRaw_1H", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_1H", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_TwoSignals =
new TH1D("rChannelActivityRaw_2", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_2", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_TwoSignals_High =
new TH1D("rChannelActivityRaw_2H", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_2H", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_ThreeSignals =
new TH1D("rChannelActivityRaw_3", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_3", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_ThreeSignals_High =
new TH1D("rChannelActivityRaw_3H", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_3H", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_MultiSignals =
new TH1D("rChannelActivityRaw_M", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_M", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
fReadoutChannelsHisto_MultiSignals_High =
new TH1D("rChannelActivityRaw_MH", "readoutChannelActivity", fReadoutChannels,
new TH1D("rChannelActivityRaw_MH", "readoutChannelActivity", fReadoutHistogramChannels,
fReadoutStartChannel, fReadoutEndChannel);
}
#endif
Expand Down Expand Up @@ -263,7 +266,8 @@ TRestEvent* TRestRawSignalChannelActivityProcess::ProcessEvent(TRestEvent* input
}
}

if (GetVerboseLevel() >= TRestStringOutput::REST_Verbose_Level::REST_Debug) fAnalysisTree->PrintObservables();
if (GetVerboseLevel() >= TRestStringOutput::REST_Verbose_Level::REST_Debug)
fAnalysisTree->PrintObservables();

return fSignalEvent;
}
Expand Down Expand Up @@ -293,3 +297,19 @@ void TRestRawSignalChannelActivityProcess::EndProcess() {
#endif
}
}

///////////////////////////////////////////////
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid implementation of InitFromConfigFile for basic std (double,string,vector) type parameters. InitFromConfigFile should be reimplemented when a complex structure is required, and some logic must be imlemented.

The present InitFromConfigFile can be automatically read by TRestEventProcess::InitFromConfigFile, thus it is not needed.

There is an additional problem, for example, in the implementation of daqChannels that fills the metadata member fDaqHistogramChannels.

If this method is not reimplemented, then the naming convention will follow: fDaqHistogramChannels --> daqHistogramChannels, or fDaqChannels will be daqChannels.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, if InitFromConfigFile is not necessary, then all this PR is also not necessary. The problem is that the values in the RML were not being read, that's why I did this.
Now doing some other tests I found that they are read but not as expected, so I'm going to try and find the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found a problem but I don't know how to solve it... Using the default values, it all works:
image

But if I introduce values manually in the RML, e.g.

<addProcess type="TRestRawSignalChannelActivityProcess" name="rawChActivity" value="ON" verboseLevel="info" >
    <parameter name="lowThreshold" value="20" />
    <parameter name="highThreshold" value="40" />
    <parameter name="daqChannels" value="400" /> 
    <parameter name="daqStartChannel" value="4000" /> 
    <parameter name="daqEndChannel" value="5000" />
    <parameter name="readoutChannels" value="250" />
    <parameter name="readoutStartChannel" value="1" />
    <parameter name="readoutEndChannel" value="300" />
 </addProcess>

then all the values are wrongly read and/or applied to the wrong variable, and some are taken from the default:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

@jgalan do you know why this might be happening?

/// \brief Function to read input parameters from the RML
/// TRestRawSignalChannelActivityProcess metadata section
///
void TRestRawSignalChannelActivityProcess::InitFromConfigFile() {
fLowThreshold = StringToDouble(GetParameter("lowThreshold", "25"));
fHighThreshold = StringToDouble(GetParameter("highThreshold", "50"));

fDaqHistogramChannels = StringToInteger(GetParameter("daqChannels", "300"));
fDaqStartChannel = StringToInteger(GetParameter("daqStartCh", "4320"));
fDaqEndChannel = StringToInteger(GetParameter("daqEndCh", "4620"));
fReadoutHistogramChannels = StringToInteger(GetParameter("readoutChannels", "128"));
fReadoutStartChannel = StringToInteger(GetParameter("readoutStartCh", "0"));
fReadoutEndChannel = StringToInteger(GetParameter("readoutEndCh", "128"));
}