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

New metadata classes to perform calibration and odds analysis over a TRestDataSet #392

Merged
merged 16 commits into from
Apr 20, 2023

Conversation

juanangp
Copy link
Member

@juanangp juanangp commented Mar 27, 2023

juanangp Large: 738

New metadata clasess have been added under framework:

  • TRestCalibration is meant to perform the calibration of a TRestDataSet, the calibration is performed over different peaks provided in the config file. If an output file name is provided a new dataset will be generated with the corresponding metadata and calibration constants, while adding a new observable, calib_energy. In addition, the spectrum and the fit results will be stored in the output file.
  • TRestOdds performs the log odds of the different observables given in the config file and for a particular dataSet. To perform the log odds first the probability density funcion (PDF) is obtained for a set of observables in the desired range and used to compute the log odds. New observables are created in the output dataSet odds_obsName and the addition of all of them in odds_total. If an input odds file is provided, the different PDFs are retrieved from the input file.

@juanangp juanangp requested a review from a team March 27, 2023 18:44
/// This class is meant to perform the calibration of different runs
class TRestCalibration : public TRestMetadata {
private:
// Name of the output file
Copy link
Member

Choose a reason for hiding this comment

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

I would leave one white space between data member definitions so that is easier to read the documentation line corresponding to the metadata member.

Also I think doxygen requires to use a triple /// comment.

/// TRestCalibration performs the calibration of a TRestDataSet, the
/// calibration is performed over different peaks provided in the config
/// file, in which the first peak should corresponds to the maximum in the
/// spectrum histogram.
Copy link
Member

Choose a reason for hiding this comment

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

I understand then that the peaks are sorted in descending energy order? First peak highest energy, latest peak the lowest energy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary, the requirement is that the first peak should correspond to the maximum in the spectra. The rest of the peaks could be randomly ordered.

@jgalan
Copy link
Member

jgalan commented Mar 27, 2023

These new classes are clearly in a new type of metadata class that uses the dataset to generate a new column on the input dataset. We should think that there might appear more classes of this type. Perhaps we should create a new directory to put those closes inside, together with TRestDataSet.

I would also think about introducing a name root to these classes, such as TRestDataSetCalibration and TRestDataSetOdds so that we clearly identify the functionality of the metadata class as soon as we see the name, and then as a user we can discover other classes.

@jgalan jgalan requested a review from a team March 27, 2023 19:13
@juanangp juanangp requested review from lobis and a team March 28, 2023 08:10
@juanangp juanangp merged commit babeff3 into master Apr 20, 2023
@juanangp juanangp deleted the calibOdds branch April 20, 2023 17:53
jgalan added a commit that referenced this pull request May 3, 2023
This reverts commit babeff3, reversing
changes made to 06ca238.
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