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

Adding new detectors - integration of RomanPot detectors for CTPPS project #13766

Merged
merged 2 commits into from Mar 21, 2016

Conversation

grzanka
Copy link
Contributor

@grzanka grzanka commented Mar 17, 2016

Integration of RomanPot (RP) detectors for CTPPS.
DetId=7 refers to the standard sequence of Totem RP + Si Strips sensors.
These detectors are required in the early runs of CTPPS (2016 start of run).
DetId=8 refers to the baseline setup of CTPPS with new 3D pixel sensors (operative end of 2016)
and Timing sensors (operative mid 2016) installed in a new Roman Pot.

The reason to have two DetId is to avoid possible clashes

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @grzanka (Leszek Grzanka) for CMSSW_8_1_X.

It involves the following packages:

DataFormats/DetId

@cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please review it and eventually sign? Thanks.
@slava77, @Degano, @smuzaffar you are the release manager for this.

cms-bot commands are list here #13028

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2016

@cmsbuild please test

@civanch I have asked for this to come in a separate PR due to its simplicity and also a large chain of dependencies to recompile, including about a half of CMSSW.
Prompt review would be nice.
Thanks.

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11950/console

@grzanka
Copy link
Contributor Author

grzanka commented Mar 17, 2016

@slava77 Will we be able to see results of Jenkins tests ? When I go to https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11950/console, I get "Authorization failed" message.

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2016

@grzanka
jenkins console just shows work in progress.
You'd be more interested to see the results when they come out (it will be a separate message from cmsbuild)
Let's see if you can open any of those (I'll send an email)

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2016

@grzanka
please edit the PR title ("Edit" button should be at the top) and add a more specific note that this is for detector enum, e.g., add ": add 2 detector enums"

@grzanka grzanka changed the title Integration of RomanPot (RP) detectors for CTPPS. Adding two new detector enums (ids) - integration of RomanPot detectors for CTPPS project Mar 17, 2016
@grzanka
Copy link
Contributor Author

grzanka commented Mar 17, 2016

@slava77 is the title better now ?

@slava77
Copy link
Contributor

slava77 commented Mar 17, 2016

On 3/17/16 1:11 PM, Leszek Grzanka wrote:

@slava77 https://github.com/slava77 is the title better now ?

Yes.
Thank you.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#13766 (comment)

@cmsbuild
Copy link
Contributor

-1

Tested at: 39f7e69
I found errors in the following unit tests:

---> test runtestTqafTopEventSelection had ERRORS

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

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
64a9f14
b0e6bb9
72b15df
5765eb1
6bb47c7
bd2081e
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13766/11950/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-13766/11950/git-merge-result

@slava77
Copy link
Contributor

slava77 commented Mar 18, 2016

runtestTqafTopEventSelection has errors in the IB. So, not related to this PR.

@cmsbuild
Copy link
Contributor

@civanch
Copy link
Contributor

civanch commented Mar 18, 2016

please test

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/11961/console

@davidlange6
Copy link
Contributor

Naively I would prefer one "detector" and then sub detectors. how many sub detectors are expected to be defined for these two new "detector"s?

@cmsbuild
Copy link
Contributor

-1

Tested at: 39f7e69
I found errors in the following unit tests:

---> test runtestTqafTopEventSelection had ERRORS

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

@civanch
Copy link
Contributor

civanch commented Mar 18, 2016

@grzanka , if it is possible, would be ideal implementing David request to have only one DetId TOTEM not two. In other cases (HCAL, for example) we have several sub-detectors as well and there are different level of upgrades.

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

Pull request #13766 was updated. @cmsbuild, @civanch, @mdhildreth, @davidlange6 can you please check and sign again.

@grzanka grzanka changed the title Adding two new detector enums (ids) - integration of RomanPot detectors for CTPPS project Adding new detectors - integration of RomanPot detectors for CTPPS project Mar 21, 2016
davidlange6 added a commit that referenced this pull request Mar 21, 2016
Adding new detectors - integration of RomanPot detectors for CTPPS project
@davidlange6 davidlange6 merged commit 37bbfd6 into cms-sw:CMSSW_8_1_X Mar 21, 2016
davidlange6 added a commit that referenced this pull request May 22, 2016
Adding new detectors - integration of RomanPot detectors for CTPPS project - backport of PR #13766
@jan-kaspar jan-kaspar deleted the ctpps_detid_change branch October 24, 2017 15:51
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

5 participants