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

TRestDetectorSingleChannelAnalysisProcess fixes #29

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Conversation

jgalan
Copy link
Member

@jgalan jgalan commented Dec 9, 2021

jgalan 22

Fixing compilation due to new protections of TRestDetectorReadout added in the parallel framework branch.

TRestDetectorSingleChannelAnalysisProcess::SaveGainMetadata has been also commented since I believe does not follow the common way to save metadata output data.

@nkx111
Copy link
Member

nkx111 commented Dec 11, 2021

I believe the readout should be already added and saved at TRestRun

When cloned, all the metadata object inside TRestRun will be lost.

Will not be the process stored anyway in the data processing chain?

The processes will be saved in the ouput file, but not the "calibration file" here. I think we need to save this process to tell the mapping configuration from the calibration file.

Why should the process set the filename? It is a data exporting function?

Yes, it is exporting a "calibration file" here.

@jgalan
Copy link
Member Author

jgalan commented Dec 13, 2021

So, you end up with two files? The usual output file after processing, and a second one for exporting the calibration data?

Why the calibration data is not just added to the usual first file?

Why this method is called at EndProcess?

So, why cannot simply do: fRun->AddMetadata( xxx ) where fRun is a pointer to the input run info metadata?

@nkx111
Copy link
Member

nkx111 commented Dec 13, 2021

So, you end up with two files? The usual output file after processing, and a second one for exporting the calibration data?

Yes

Why the calibration data is not just added to the usual first file?

Because I don't want to save calibration data with event data or observables.

Why this method is called at EndProcess?

It means that after the process chain the process will summarize all its processed data, do the fitting, and generate the calibration file.

So, why cannot simply do: fRun->AddMetadata( xxx ) where fRun is a pointer to the input run info metadata?

Because I don't want to save calibration data with event data or observables.

@jgalan
Copy link
Member Author

jgalan commented Jan 5, 2022

Please @nkx, aprouve and merge this PR to fix the framework merge.

@@ -240,6 +239,13 @@ void TRestDetectorSingleChannelAnalysisProcess::FitChannelGain() {
}
}

/*** This should not be done. The framework saves any metadata structure inside of the
Copy link
Member

Choose a reason for hiding this comment

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

Here I am exporting a calibration file containing TRestDetectorGainMap metadata object. I think a calibration file should not contain observables. Yes, newing a TRestRun object is bad. I can use TRestRun of the process to store calibration data.

@nkx111
Copy link
Member

nkx111 commented Jan 5, 2022

I though the discussion was not over. So is it OK to use TRestRun to store another file from the process chain?

@jgalan jgalan requested a review from nkx111 January 5, 2022 12:59
@jgalan
Copy link
Member Author

jgalan commented Jan 5, 2022

I though the discussion was not over. So is it OK to use TRestRun to store another file from the process chain?

I see, you are creating an independent file that contains only the objects you are interested in , i.e. ,fRun, fCalib, fReadout, and the process itself.

    fCalib = new TRestDetectorGainMap();
     fCalib->fChannelGain = fChannelGain;
     fCalib->SetName("ChannelCalibration");

     TRestRun* r = new TRestRun();
     TRestRun* r = (TRestRun*)fRunInfo->Clone();
     r->SetOutputFileName(filename);
     r->AddMetadata(fCalib);
     r->AddMetadata(fReadout);
     r->AddMetadata(this);
     r->FormOutputFile();

Note that the process itself contains the information of calibration map. Perhaps it would be better that TRestDetectorGainMap would be a metadata member of the process?

Of course, I have nothing against exporting data this way (just becoming worried about traceability because the output file becomes quite similar to an official REST file - perhaps it would be good to have a metadata member inside TRestRun that tells us that it is an exported file? But of course, it has no eventdata, I guess it is no problem). But in any case, all the calibration data is going to be included at the final output file, right? Inside fChannelGain.

I dont see a problem, but perhaps we could have a method at TRestRun that allows to export "metadata" pure files.

TRestRun::Export( string filename, std::vector <TRestMetadata *> )

or

TRestRun::Export(string filename, string saveMetadataObjects="TRestDetectorReadout,TRestDetectorCalibrationGain,TRestDetectorSingleChannelAnalysisProcess")

where those classes are classes known to TRestRun.

@jgalan
Copy link
Member Author

jgalan commented Jan 5, 2022

On the other hand, if the purpose of your processing is only to generate that calibration map. Why not add a feature at TRestRun, that allows you to chose what are finally the objects you want to be stored in the disk?

Kind of "pureAnalysis" option we had before, but more specific to "pureMetadata" where you select the metadata classes you are interested to register. In that way, TRestRun will know that you asked explicitly for that.

@jgalan
Copy link
Member Author

jgalan commented Jan 7, 2022

Ok, I just merge this PR since I believe it is not a problem for other parts of the code. I created a new issue to track the possibility of exporting using a unique common method. #30

@jgalan jgalan merged commit 0400979 into master Jan 7, 2022
@jgalan jgalan deleted the nkx111-patch-2 branch January 7, 2022 13:38
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.

2 participants