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.

Sign up for GitHub

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

Jump to bottom

HGCal trigger geometry v4 #113

Open
wants to merge 12 commits into
base: hgc-tpg-devel-CMSSW_14_0_0_pre1
Choose a base branch
from
Open

HGCal trigger geometry v4 #113

wants to merge 12 commits into from
+126,471 −1,013

Conversation

EmyrClement
Copy link

PR description:

This PR introduces v4 of the trigger geometry, which is based on the "xml mapping files" derived by Andy (see mapping repo in gitlab).

  • I have added helper functions for parsing the xml files under L1Trigger/L1THGCal/interface/mappingTools and L1Trigger/L1THGCal/src/mappingTools. These are based on code provided by Andy, but modified to use Xerces-C (as opposed to pugi-xml) as the xml parser, which is available in CMSSW. Perhaps these tools should be encapsulated into a helper class rather than a set of functions.
  • The new v4 geometry class is L1Trigger/L1THGCal/plugins/geometries/HGCalTriggerGeometryV9Imp4.cc, which is almost identical to the V3 implementation, however the fillMaps function has been modified to fill the relevant data members (e.g. maps like stage2_to_stage1links_) with information from the xml files using the helper functions described in the previous bullet.
  • xml files added for now to L1Trigger/L1THGCal/data, which will ultimately be located in cms-data. I have added files for all possible scenarios, taken from the mapping repository in ~Feb 2024.

PR validation:

So far, I have checked that the consistency of the v4 geometry using the existing geometry tester for the v3 geometry, given the interfaces of the two trigger geometries are almost the same. There are some caveates:

  • The v4 geometry doesn't contain the number of elinks per module, so I have temporarily disabled any check involving those.
  • There is an issue with the mapping of Scintillator trigger cells to modules, so I have also temporarily disabled those checks.
  • When performing a consistency check of Stage-1 FPGAs to modules, there is sometimes a mismatch in the module type between that taken from the HGCal geometry and that found when looking for the corresponding module connected to a particular Stage 1 FPGA. The module layer, u, v, and subdetector match, however the module type (i.e. high/low denisty and thickness) differs.

I will continue with some further validation using the existing emulators in CMSSW.

@jbsauvan
Copy link

Thanks @EmyrClement !
A couple of general comments:

  • It may be better to avoid as much as possible code duplication between V3 and V4. If only the map filling differs, then it would make sense to make these two classes derive from an intermediate virtual class with most of the functionalities. The V3 and V4 implementations would then only implement the map filling.
  • The mapping tool functions could eventually be integrated within the v4 geometry implementation, as I suppose they are specific to this xml mapping file format and are not generic functions that could be used in other parts of our code base.
  • Concerning the mapping files to be added, I would only put for the moment those that are needed/used by the code. For instance the mif files shouldn't be needed, and many of the xml files as well.

@jbsauvan
Copy link

On a related topic, I have finalized a preliminary version of the updated geometry testing tools, which I plan to PR Today.

@EmyrClement
Copy link
Author

Thanks @jbsauvan for the comments, I in principle agree with all of them, and can likely integrate the first and third comment in this PR. For your last comment, my intention was to include all xml files that are required by CMSSW for any possible scenario, and then exclude any additional files within these directories that are produced when deriving the mappings (e.g. pdf files with validation diagrams/summary of each mapping scenario). There are some files I overlooked (e.g. the mif file you mention), so will remove those. Beyond that, is it worth keeping one entire consistent set of xml files for all scenarios, or should we just keep a reduced set/just one scenario?

@jbsauvan
Copy link

is it worth keeping one entire consistent set of xml files for all scenarios, or should we just keep a reduced set/just one scenario?

I would rather tend to prefer being minimal. There is only one scenario that is considered at the moment by firmware designers. And within a scenario not all the files are currently being used, so I would only add the needed file and things can be added later when necessary. We just have to make sure that the version of these files is clearly identified so that they can be tracked down to their origin.

@jbsauvan jbsauvan linked an issue Jul 22, 2024 that may be closed by this pull request
Integrate new BC+STC mappings #84
Open
@EmyrClement EmyrClement mentioned this pull request Oct 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
emulator geometry
Projects
HGCAL TPG Simulation Developments
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Integrate new BC+STC mappings
2 participants
@EmyrClement @jbsauvan