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

[12_4_X] PPS reco patch for bad pot #38581

Merged
merged 9 commits into from Jul 15, 2022
Merged

Conversation

fabferro
Copy link
Contributor

@fabferro fabferro commented Jul 1, 2022

PR description:

This PR provides a patch to be able to produce tracks inside PPS roman pot 45-220-fr that has only 2 planes working out of 6 (as to the current date). The code creates tracks using two hits (one per plane) drawing a straight line between them. The track is required to be isolated and horizontal. The goal is to recover at least some proton tracks for alignment/debugging purposes.
The patch is enabled by means of a runtime flag. Run2 processes are protected via Era mechanism.

PR validation:

Run2 data show no difference. Preliminary run3 data show recovered tracks as expected.

This PR is a backport of PR#38553.

@jpata @grzanka @AndreaBellora

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 1, 2022

A new Pull Request was created by @fabferro (Fabrizio Ferro) for CMSSW_12_4_X.

It involves the following packages:

  • RecoPPS/Local (reconstruction)

@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks.
@grzanka this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

@qliphy
Copy link
Contributor

qliphy commented Jul 2, 2022

please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 2, 2022

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f1f495/25942/summary.html
COMMIT: d9e578a
CMSSW: CMSSW_12_4_X_2022-07-01-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38581/25942/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 0 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3675663
  • DQMHistoTests: Total failures: 2
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 3675639
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@jpata
Copy link
Contributor

jpata commented Jul 2, 2022

@fabferro there are no reco changes detected by the bot (CTPPS stuff maybe not tracked?), but does this change Run3 MC in any way? Does it need to be protected with a modifier?

@fabferro
Copy link
Contributor Author

fabferro commented Jul 2, 2022

@fabferro there are no reco changes detected by the bot (CTPPS stuff maybe not tracked?), but does this change Run3 MC in any way? Does it need to be protected with a modifier?

Run3 MC are effected in the sense that if the Simulation simulates PPS with all working planes, then unneeded 2-hit tracks will be added in the track collection. They can be easily identified and taken care of at analysis level.
That said, the scope of this change is limited to data reconstruction till HW repair can be done.
So, in principle, this patch should not enabled when reconstructiong MC.
@jpata Can this distinction (MC vs Data) be done via Eras?

@qliphy
Copy link
Contributor

qliphy commented Jul 5, 2022

backport of #38553

@jpata
Copy link
Contributor

jpata commented Jul 5, 2022

Can this distinction (MC vs Data) be done via Eras?

It doesn't seem like this is supported. Perhaps it would be better to create a new era that is only enabled when data is processed? Note that this has to be done in both the master version and this backport.

@fabferro
Copy link
Contributor Author

fabferro commented Jul 5, 2022

Can this distinction (MC vs Data) be done via Eras?

It doesn't seem like this is supported. Perhaps it would be better to create a new era that is only enabled when data is processed? Note that this has to be done in both the master version and this backport.

If it's not supported, I'd say that PPS simulation can live with this patch unitl the detectors are repaired and the patch definitely disabled. The 2-hit tracks that are created reconstructing simulated data can be easily recognized and dealt with at analysis level.

@perrotta
Copy link
Contributor

perrotta commented Jul 5, 2022

@fabferro I think that the actual (time dependent) detector configuration should be available in the DB, and the reconstruction should act according to that configuration, irrespectfully of being real data or MC. Hybrid cases as "I have a description in the DB, but I let my reconstruction operate as if it was different" are only expected to lead to confusion and possible mistakes.

@tvami
Copy link
Contributor

tvami commented Jul 5, 2022

@fabferro
Copy link
Contributor Author

fabferro commented Jul 6, 2022

@fabferro I think that the actual (time dependent) detector configuration should be available in the DB, and the reconstruction should act according to that configuration, irrespectfully of being real data or MC. Hybrid cases as "I have a description in the DB, but I let my reconstruction operate as if it was different" are only expected to lead to confusion and possible mistakes.

@perrotta @tvami @jpata
In order to use the DB we can profit of the CTPPSPixelAnalysisMask. With it we can mask the parts of detector not working and reproduce the same configuration both in data and simu processes at reco level. We just need to update the tags, the GT and link the new GT in the next release.

Another way not to disrupt the recostruction of simulated data would be put if(run==1) isBadPot = false; somewhere. A bit rough but probably effective. It would disappear once that the patch is not needed anymore.

@davidlange6
Copy link
Contributor

davidlange6 commented Jul 6, 2022 via email

@fabferro
Copy link
Contributor Author

fabferro commented Jul 6, 2022

On Jul 6, 2022, at 12:35 PM, Fabrizio Ferro @.***> wrote: Another way not to disrupt the recostruction of simulated data would be put if(run==1) isBadPot = false; somewhere. A bit rough but probably effective. It would disappear once that the patch is not needed anymore.
Eg, you will instead have if (run>x and run<y)?

@davidlange6 you mean something like if(run>first.run3.run && run<99999999) isBadPot=true; ?
It can work as well. Then 999999999 will be replaced with last.run3.run.with.bad.pot once we have repaired the pot.

@tvami
Copy link
Contributor

tvami commented Jul 6, 2022

Why dont we just do the tag update? In that way we are independent of the release schedule. For example I understand that 12_4_X is to be cut today, and this PR will not be in it... (given that it's master is not merged yet)

@cmsbuild
Copy link
Contributor

Pull request #38581 was updated. @emanueleusai, @ahmad3213, @cmsbuild, @jfernan2, @clacaputo, @jpata, @pmandrik, @micsucmed, @rvenditti can you please check and sign again.

@tvami
Copy link
Contributor

tvami commented Jul 12, 2022

type bugfix,ctpps

@clacaputo
Copy link
Contributor

@cmsbuild please test

@cmsbuild
Copy link
Contributor

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f1f495/26200/summary.html
COMMIT: 859f6cb
CMSSW: CMSSW_12_4_X_2022-07-13-1100/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/38581/26200/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • No significant changes to the logs found
  • Reco comparison results: 2 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 3676111
  • DQMHistoTests: Total failures: 7
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 3676081
  • DQMHistoTests: Total skipped: 22
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.004 KiB( 49 files compared)
  • DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
  • Checked 208 log files, 45 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@clacaputo
Copy link
Contributor

+reconstruction

@fabferro
Copy link
Contributor Author

@perrotta is this being merged?

@qliphy
Copy link
Contributor

qliphy commented Jul 14, 2022

@perrotta is this being merged?

@fabferro The PR is master was just merged. Once the IB test passes without any problem, we can merge this one. In the meantime, you can also work on the further cleanups as suggested in the master PR.

@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 CMSSW_12_4_X IBs (tests are also fine) and once validation in the development release cycle CMSSW_12_5_X is complete. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

@qliphy
Copy link
Contributor

qliphy commented Jul 15, 2022

+1

@cmsbuild cmsbuild merged commit 69f091e into cms-sw:CMSSW_12_4_X Jul 15, 2022
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.

None yet

9 participants