Skip to content

[EMCAL-566] Add executable for performing time and bad cell calibration#7886

Merged
shahor02 merged 2 commits intoAliceO2Group:devfrom
jokonig:EMCAL-566
Jan 11, 2022
Merged

[EMCAL-566] Add executable for performing time and bad cell calibration#7886
shahor02 merged 2 commits intoAliceO2Group:devfrom
jokonig:EMCAL-566

Conversation

@jokonig
Copy link
Copy Markdown
Collaborator

@jokonig jokonig commented Dec 16, 2021

  • executable to run calibration, both time and bad channel, in offline
    mode with local root histograms. Goal is to validate and debug the
    calibration using run2 data.

  • modified root to boost histogram conversion function: Specified the
    return type, corrected bin numbers for boost histograms

  • Added OpenMP option to cmake list. Goal is to parallelize the
    time-calibration as many independent fits are performed

  • Added implementation of time calibration in EMCALCalibExtractor

[EMCAL-630] Force usage of references in range-based iteration over e…
Copy link
Copy Markdown
Collaborator

@mfasDa mfasDa left a comment

Choose a reason for hiding this comment

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

I think something entered in the CMakeLists by accident that needs to be fixed now. For what concerns the OMP thread allocation I guess this can be handled in the next iterations.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Was probably unintended

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The "TARGETVARNAME targetName" is needed for OpenMP. Based on O2/Detectors/TOF/calibration/CMakeLists.txt

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

17644 threads is probably out of order for local machines :) Needs to be checked with @shahor02 and @chiarazampolli where to put the maximum here, I guess omp threads will need to be shared among components.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes NCells is unrealistic. But like this it will use all available resources when calling the omp_get_max_threads(), right?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We might want to use

o2::base::NameConf::getCCDBServer();

introduced in #7778 as the default server path.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@sawenzel @shahor02 Currently the o2::emcal::CalibDB is still using o2::ccdb::CcdbApi as underlying CCDB API. In principle it would be nice to migrate to o2::ccdb::BasicCcdbManager for the CCDB access, but the BasicCcdbManager does not provide write functions, which is i.e. used in this case. Do you see a problem adding a function storing objects which forwards to StoreAsTFileAny to the BasicCcdbManager? The goal of the o2::emcal::CalibDB is to provide a specific interface for EMCAL CDB access so users don't need to mess around with paths.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mfasDa the custom access to the CCDB from the user code in the workflows should be anyway outphased once the DPL CCDB service becomes functional.

@jokonig jokonig force-pushed the EMCAL-566 branch 4 times, most recently from 05f1db3 to 1cacd4f Compare December 23, 2021 16:45
Comment on lines 237 to 239
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I suspect this is to circumvent the problem with the multiple definition of TFormula when running with OMP threads > 1. This could become tricky in a multi-threaded environment. As far as I understand the variables should be allocated per thread (thread-local storage) based on our build settings. However would be good if we could find a way to avoid making members static.

@shahor02
Copy link
Copy Markdown
Collaborator

@jokonig could you please apply changes suggested by the fullCI (braces)?

- executable to run calibration, oth time and bad channel, in offline
mode with local root histograms. Goal is to validate and debug the
calibration using run2 data.

- modified root to boost histogram conversion function: Specified the
return type, corrected bin numbers for boost histograms

- Added OpenMP option to cmake list. Goal is to parallelize the
time-calibration as many independent fits are performed

- Added implementation of time calibration in EMCALCalibExtractor
@mfasDa
Copy link
Copy Markdown
Collaborator

mfasDa commented Jan 10, 2022

@shahor02 Should now be ready for merge.

@shahor02 shahor02 merged commit 45973e4 into AliceO2Group:dev Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants