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

PPS Timing Detector: Updated mapping for 2018 #22404

Merged
merged 3 commits into from Mar 21, 2018

Conversation

nminafra
Copy link
Contributor

@nminafra nminafra commented Mar 1, 2018

We are updating the mapping of the PPS Timing Diamond detector to reflect the different cabling done during YETS.
During 2017 we had some bugs in the front-end electronics that were fixed with the Digitizer Board V3. Moreover, the new cabling is optimized to balance the occupancy in the HPTDCs to not saturate the read-out fifos.
The cabling will not change anymore during 2018.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

The code-checks are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

A new Pull Request was created by @nminafra (Nicola Minafra) for master.

It involves the following packages:

CondFormats/CTPPSReadoutObjects
EventFilter/CTPPSRawToDigi

@perrotta, @ghellwig, @arunhep, @cerminar, @cmsbuild, @franzoni, @slava77, @ggovi, @lpernie can you please review it and eventually sign? Thanks.
@ghellwig, @forthommel, @tocheng, @Martin-Grunewald, @mmusich, @seemasharmafnal this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos you are the release manager for this.

cms-bot commands are listed here

@perrotta
Copy link
Contributor

perrotta commented Mar 1, 2018

@nminafra @fabferro : cannot this be made available through a GT, as it was done for the other #22264 CTPPS pull request?

@fabferro
Copy link
Contributor

fabferro commented Mar 1, 2018

@perrotta: you are right Andrea. Unfortunately what you ask had to be postponed due to higher priorities involving the same people (like @nminafra).

@slava77
Copy link
Contributor

slava77 commented Mar 1, 2018

@cmsbuild please test

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/26412/console Started: 2018/03/01 21:46

@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

-1

Tested at: 0275564

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-22404/26412/summary.html

I found follow errors while testing this PR

Failed tests: Build

  • Build:

I found an error when building:

gmake[1]: Target 'PostBuild' not remade because of errors.
gmake[1]: Leaving directory '/build/cmsbld/jenkins/workspace/ib-any-integration/CMSSW_10_1_X_2018-03-01-1100'
config/SCRAM/GMake/Makefile.rules:2097: recipe for target 'src' failed
gmake: *** [src] Error 2
gmake: Target 'all' not remade because of errors.
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2


@cmsbuild
Copy link
Contributor

cmsbuild commented Mar 1, 2018

Comparison not run due to Build errors (RelVals and Igprof tests were also skipped)

mappingFileNames = cms.vstring("CondFormats/CTPPSReadoutObjects/xml/mapping_timing_diamond_2017.xml"),
maskFileNames = cms.vstring()
)
# 2018
Copy link
Contributor

Choose a reason for hiding this comment

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

there is a comma missing above

@slava77
Copy link
Contributor

slava77 commented Mar 1, 2018

ehm , the compilation log is more descriptive than the snippet provided by the bot:

Compiling src/EventFilter/CTPPSRawToDigi/python/ctppsRawToDigi_cff.py ...
  File "src/EventFilter/CTPPSRawToDigi/python/ctppsRawToDigi_cff.py", line 76
    cms.PSet(
      ^
SyntaxError: invalid syntax

@smuzaffar please check if the cms-bot error text selection can be improved in this case.
Thank you.

@perrotta
Copy link
Contributor

perrotta commented Mar 2, 2018

@nminafra @fabferro : please tell us the schedule foreseen for moving the PPS Timing mappings into the DB. Given the experience you gained with the implementation of a similar move in the other PR I expect that it could be done before pre3: but let us know if this is not the case, and when you plan to do it if so

@fabferro
Copy link
Contributor

fabferro commented Mar 2, 2018

@perrotta: we are installing the detectors these days. That means that the same people who should develop the code are now busy on hardware. I don't expect anything new about this subject for pre3 (next Monday) but I let @nminafra answer on this.

@nminafra
Copy link
Contributor Author

nminafra commented Mar 2, 2018

@perrotta We'll try to do our best, but I cannot guarantee to be able to implement and test it by Monday.

@slava77
Copy link
Contributor

slava77 commented Mar 2, 2018

Given the experience you gained with the implementation of a similar move

@perrotta
there is little similarity, actually.

If we migrate the mapping, it has to be done both for strips and diamonds.

@nminafra
Copy link
Contributor Author

nminafra commented Mar 5, 2018

@perrotta we started the "procedure" but we'll not be able to do all the proper tests by today.
However, the updated mapping is very important from day 1 for the commissioning of the detectors during the first low(er) luminosity runs.

Thanks

@perrotta
Copy link
Contributor

perrotta commented Mar 5, 2018

+1

  • Added the mapping of the PPS Timing Diamond detector for the 2018 setup, which is accessed within the corresponding validity range

@arunhep
Copy link
Contributor

arunhep commented Mar 13, 2018

@nminafra why do we need an xml file for the mapping? Don't we have a corresponding record in release?

@nminafra
Copy link
Contributor Author

@arunhep We changed the mapping during YETS, therefore the old mapping is still needed for 2017 data. A similar procedure was used for Timing in 2017 and for Tracking in 2016 and 2017.
We plan a migration to db as soon as we have resources.

@arunhep
Copy link
Contributor

arunhep commented Mar 13, 2018

@nminafra pardon my ignorance, is this CTPPSPixelDAQMappingRcd is different than the mapping discussed in this PR?

@nminafra
Copy link
Contributor Author

@arunhep No problem at all. This PR involves only the timing detectors which are "similar" to Strip detectors. On the contrary, CTPPSPixel have a separate code, from unpacker to reco

@arunhep
Copy link
Contributor

arunhep commented Mar 13, 2018

+1

@fabiocos
Copy link
Contributor

@fabferro @nminafra I assume this is needed in the commissioning phase of the detector, could you please comment? When is the move to GT expected to happen?

@fabiocos
Copy link
Contributor

@ggovi could you please review this PR?

@nminafra
Copy link
Contributor Author

@fabiocos This is needed as soon as possible. Without the updated mapping we cannot use the DQM and it will slow down every commissioning operation.
Moving to the GT is foreseen after the commissioning: we have very limited resources and the same people were doing the installation and they'll be involved in the commissioning.

@ggovi
Copy link
Contributor

ggovi commented Mar 20, 2018

+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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2)

@fabiocos
Copy link
Contributor

+1

@cmsbuild cmsbuild merged commit e2baf1a into cms-sw:master Mar 21, 2018
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

8 participants