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

Final DQM plot updates for the Totem T2 in the special run 25.6, to be ready for special TOTEM release testing around 12-13.6 #41922

Merged
merged 5 commits into from
Jun 13, 2023

Conversation

oljemark
Copy link
Contributor

PR description:

For the TOTEM special foreseen on 25.6.2023, the new T2 telescope will be used (and removed after the run) to measure inelastic rate and veto diffractive p+p collisions, when measuring the elastic p+p cross section.
This PR is for a special TOTEM release to be ready for testing start of next week (#41859 (comment)).

After the merging of PR #41859 and submission of backport PR #41916 , T2 Digis are successfully saved in the event and used to produce Digi-based DQM plots in common reconstruction with the rest of the PPS detectors.
In this PR, the DQM plots that use the T2Segmentation helper (removed due to T2 RecHit and TotemGeometry dependence causing relVal issues detailed in the previous PR's) were added back with a hardcoded "geometry" mapping from channel number to (X,Y) position using the attached mapping on slide 1 (agreed with @vavati & E. Bossini to use slide 1, not slide 3), see comments here: master...CTPPS:cmssw:fo_nt2_dqm_final#diff-c26415cb77d83b1dbe3bdea47ca041d0e35f6ab203364c54d92c4b143b11ce47R47

T2-map-latest-2023jun11.pdf

Several other T2 plots were also adjusted as to the T2 Digi demands used.

This PR is the final one needed for testing a special 13_0_X release for TOTEM, or possibly to be backported to 13_1_X too, see #41916 (comment)

PR validation:

After the inclusion of PR #41916 in master, there were no other dependent packages brought in by checkdeps , so I ran successfully a PPS workflow (1044) & the DQM test file totemt2_dqm_test_common_cfg.py , which produced the expected plots.

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

This will hopefully be backported very soon (during the coming week) to 13_0_X, and possibly later to 13_1_X, as noted above. I will submit a backport PR later with the commits from PR #41916 and the ones from this PR.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41922/35880

  • This PR adds an extra 24KB to repository

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @oljemark for master.

It involves the following packages:

  • DQM/CTPPS (dqm)

@nothingface0, @emanueleusai, @cmsbuild, @pmandrik, @syuvivida, @tjavaid, @micsucmed, @rvenditti can you please review it and eventually sign? Thanks.
@fabferro, @grzanka this is something you requested to watch as well.
@perrotta, @dpiparo, @rappoccio you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

please test

@perrotta
Copy link
Contributor

@oljemark please specify whether this PR has to be included ONLY for the special Totem run, or if it can remain in the release also for the "normal" pp runs

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-ff842c/33089/summary.html
COMMIT: ce36477
CMSSW: CMSSW_13_2_X_2023-06-11-2300/el8_amd64_gcc11
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/41922/33089/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially added 5 lines to the logs
  • Reco comparison results: 12 differences found in the comparisons
  • DQMHistoTests: Total files compared: 48
  • DQMHistoTests: Total histograms compared: 3190910
  • DQMHistoTests: Total failures: 6
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3190882
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 47 files compared)
  • Checked 207 log files, 159 edm output root files, 48 DQM output files
  • TriggerResults: no differences found

@vavati
Copy link

vavati commented Jun 12, 2023

@oljemark please specify whether this PR has to be included ONLY for the special Totem run, or if it can remain in the release also for the "normal" pp runs

T2-DQM is included only in a special sequence which is not executed in regular runs. At worse it creates empty plots. Hence whatever is easier for you: it can be included only for the special run or stay in.

@perrotta
Copy link
Contributor

@oljemark please specify whether this PR has to be included ONLY for the special Totem run, or if it can remain in the release also for the "normal" pp runs

T2-DQM is included only in a special sequence which is not executed in regular runs. At worse it creates empty plots. Hence whatever is easier for you: it can be included only for the special run or stay in.

Thank you @vavati
@emanueleusai please suggest how to proceed here. If the issue of the empty plot is not relevant, or (probably even better) if they can be enabled/disabled with a flag, keeping the related code in the cmssw release is the easiest way to maintain and resume this code when needed

@vavati
Copy link

vavati commented Jun 12, 2023

Thank you @vavati @emanueleusai please suggest how to proceed here. If the issue of the empty plot is not relevant, or (probably even better) if they can be enabled/disabled with a flag, keeping the related code in the cmssw release is the easiest way to maintain and resume this code when needed

FYI this is the sequence which is activated only in special runs:
https://github.com/cms-sw/cmssw/blob/master/DQM/Integration/python/clients/ctpps_dqm_sourceclient-live_cfg.py#LL94

@perrotta
Copy link
Contributor

urgent

oljemark added a commit to CTPPS/cmssw that referenced this pull request Jun 12, 2023
@vavati
Copy link

vavati commented Jun 12, 2023

In case:
https://cmsoms.cern.ch/cms/runs/report?cms_run=368805&cms_run_sequence=GLOBAL-RUN
has T2 emulated data + Totem strips. Good for tests.

@emanueleusai
Copy link
Member

Personally, I'd rather have the sequence activated/deactivated with a flag rather than having empty plots, but if it's urgent for the special run on 25.6, I'm fine with keeping it like this and add a flag switch later on.

@emanueleusai
Copy link
Member

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

Copy link
Contributor

@perrotta perrotta left a comment

Choose a reason for hiding this comment

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

Just a suggestion for a possible follow up PR

Comment on lines +356 to +357
const TotemT2DetId secId(detid.armId());
if (digi.hasLE()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TotemT2DetId secId(detid.armId());
if (digi.hasLE()) {
if (digi.hasLE()) {
const TotemT2DetId secId(detid.armId());

@perrotta
Copy link
Contributor

Personally, I'd rather have the sequence activated/deactivated with a flag rather than having empty plots, but if it's urgent for the special run on 25.6, I'm fine with keeping it like this and add a flag switch later on.

I agree.
@oljemark while we merge now this PR in master, in order to have it available for the special Totem run of June 25, could you please take care of shielding whatever is specific for those special runs behind some switch that can be activated via a configuration parameter (in DQM and in the other parts of the code as well)?

@perrotta
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit 681728f into cms-sw:master Jun 13, 2023
@oljemark
Copy link
Contributor Author

Hi, @perrotta . Should that 2nd PR be added in master only, or also to the backport #41923 ?

@perrotta
Copy link
Contributor

Hi, @perrotta . Should that 2nd PR be added in master only, or also to the backport #41923 ?

Let prepare it for master. The release for next week run sits on a dedicated branch, and therefore protections for the general data taking have a low priority for it

cmsbuild added a commit that referenced this pull request Jun 13, 2023
Backport of final DQM plot updates for Totem T2 run 25.6 (PR #41922)
@grzanka
Copy link
Contributor

grzanka commented Jun 14, 2023

Hi, @perrotta . Should that 2nd PR be added in master only, or also to the backport #41923 ?

Let prepare it for master. The release for next week run sits on a dedicated branch, and therefore protections for the general data taking have a low priority for it

@perrotta I has some troubles to get good connection during ORP meeting yesterday.

As for DQM guard for T2 plots: we already have a protection mechanism implemented.
If DQM runs in a standard mode then the nT2 DQM producer is not added to the DQM live config.

Only if we switch the PPS DQM to "calibration mode", the T2 DQM producer is activated.

This effectively works as a "guarding flag"

Take a look here:
https://github.com/CTPPS/cmssw/blob/0a38b0798473b67bf0e3e09296a128103c9ba8c7/DQM/Integration/python/clients/ctpps_dqm_sourceclient-live_cfg.py#L94

We are considering further developments of fine control of DQM and reconstruction tasks for T2 at a later stage.
Anyway we will need to deal with retrieving T2 geometry from the CondDB record and adding another reconstruction steps (to be used for later rereco of collected data). At that place we could have a fine control of T2 reconstruction and DQM options.

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

Successfully merging this pull request may close these issues.

6 participants