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

Proposed new API for PixelBarrelName and PixelEndcapName #17

Open
wants to merge 7 commits into
base: removeSiStripSubDetId_920p3
Choose a base branch
from

Conversation

pieterdavid
Copy link
Owner

After having a look at the PixelBarrelName and PixelEndcapName classes (and only a mild headache ;-) ), a proposal for changing these classes while migrating from PXBDetId and PXFDetId to TrackerTopology (in this case: completing the migration - it looks like the methods for that are mostly in place, but not used everywhere yet).

What I would mostly change is that the PixelModuleName base class could store the DetId and use that as a unique identifier - everything else is additional information (calculated at construction and stored, or calculated on demand, using the TrackerTopology and a phase0/1/... switch passed to the constructor).
The constructor from DetId and TrackerTopology then does not need to change, the one from the separate fields is not necessary (the DetId can be constructed using the TrackerTopology and the above), and the constructor from std::string is replaced by a static method fromName, that does the parsing (as before) and constructs a DetId from that. The constructor without TrackerTopology disappears, but on the other hand the getDetId(const TrackerTopology*) const method is not needed any more, and getDetId() const and operator==(const PixelModuleName& other) const are non-virtual methods of PixelModuleName.
The PixelEndcapNameUpgrade and PixelBarrelNameUpgrade classes seem to be the phase1-only versions, so they can be removed in favour of the more general ones.

As for migration... since the constructor from DetId, const TrackerTopology* and bool exists already, first step would be to change all uses of the non-parsing constructors ((DetId, bool) and from the list of fields), not changing this class yet. Changing the uses of the constructor from std::string and the few (if any) of getDetId(const TrackerTopology*) could be part of another PR changing these classes. For a full list of all uses, see https://twiki.cern.ch/twiki/bin/view/CMS/SiStripLocalReco_notesSiStripSubDetIdToTrackerTopologyMigration#Remaining_PXBDetId_and_PXFDetId
In case we want to change the phase0/phase1 bool to an enum, that should happen everywhere at the same time.

Feedback welcome...

@pieterdavid
Copy link
Owner Author

@boudoul @mmusich @dimattia : does this make sense? Do you agree with the proposed strategy? If yes we can translate it into a list of actions and divide the work.

CC @OlivierBondu @alesaggio @vidalm
(I don't intend to "merge" this PR here, but it was the easiest way to have a place to discuss the code changes ;-) )

@boudoul
Copy link

boudoul commented Sep 19, 2017

hello @pieterdavid thanks for sharing , I should propagate to also customers who are the pixel team. However, until end of this month (september) they are fully (over) busy with the preparation of the re-reco etc.. which is top priority so I prefer not to adress this issue exactly at this particular moment . So I may ask you to wait a bit for the ongoing pixel peak activity to decrease a bit - Thanks for your undertanding (and the work !!)

@pieterdavid
Copy link
Owner Author

Thanks @boudoul , sure, this can wait a bit, we are not in a hurry.

@boudoul
Copy link

boudoul commented Sep 19, 2017

thanks @pieterdavid , I will anyway try to look myself at your proposal meanwhile

@boudoul
Copy link

boudoul commented Sep 21, 2017

I take the liberty to add @gflouris and @fioriNTU since DQM in one of the customer of those classes

pieterdavid pushed a commit that referenced this pull request Jul 17, 2018
Update branch CMSSW_10_2_X to cms-sw
pieterdavid pushed a commit that referenced this pull request May 29, 2020
…ms (L1Trigger/TrackFindingTMTT) (cms-sw#29381)

* create separate PRs for the two L1TK packages

* Improved KF efficiency at high eta

* Moved MC data files to cms-data

* Removed old file

* Removed KF HLS to put instead in external library

* Ran scram b code-format

* Delete KF4ParamsComb.h.bak

* Delete KF4ParamsCombIV.bak

* Delete KF4ParamsCombV2.bak

* Delete KF5ParamsComb.h.bak

* Delete KF4ParamsComb.cc.bak

* Delete KF4ParamsCombIV.bak

* Delete KF4ParamsCombV2.bak

* Delete KF5ParamsComb.cc.bak

* L1 tk integration tmtt pre5 (#7)

* Added CMS code style fixes

* Removed old file

* Reapplied stub b code-format

* All code review changes (#13)

* Fix clang errors (#14)

* fixed clang error

* directory for MC txt files

* Fixed clang warnings + minor simplifications (#15)

* tweak

* tweak

* Fixed clang warnings and small simplifications

* Fixed clang warnings and small simplifications

* All remaining review comments addressed (#16)

* Replaced vector size with empty function

* Simplified DegradeBend and StubWindowSuggest

* Fixed more review comments

* More review comments

* code reformat

* Ran exhaustive clang tidy

* Added library to BuildFile.xml (#17)

* Deleted TrackFindingTMT/data/README (#18)

* Added library to BuildFile.xml (This was already done yesterday. Not sure why it appears again)

* README file in data directory deleted

* Fix review comments (#20)

Co-authored-by: Louise Skinnari <[email protected]>
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.

2 participants