cms-l1t-offline / cmssw

CMS Offline Software
cms-sw.github.io/cmssw
Apache License 2.0
17 stars 27 forks source link

Fix module names in HGCal tower customisation function #1211

Closed EmyrClement closed 9 months ago

EmyrClement commented 9 months ago

PR description:

The module names in the HGCal tower customisation function required by the GCT calo jets/taus #1174 is assuming the old naming convention of the l1t modules. I assume this customisation isn't used in any test workflow, which is why it's not been noticed before.

PR validation:

Successfully run standard L1 cmsDriver command with this customisation function.

EmyrClement commented 9 months ago

I also have a related question on how to handle the use of this customisation, which the GCT calo jets/taus require. For the upcoming reviews, we could include the customisation in the standard cmsDriver command that's used? Or should we make this change explicit in the L1 sequence so you can't forget to apply it?

The other complication is that the NN Calo Taus may require the original HGCal towers. They are updating their training to adapt to use the same towers as the GCT calo taus, but we may need to produce two collections of HGCal towers, one with and one without the modifications introduced by the customisation. I think in this case we would need to have two sequences producing the two tower collections, rather than a customisation function that switches between the two.

triggerDoctor commented 9 months ago

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

Attempts to compile this PR succeeded!

Info Value
return code 0
command evalscramv1 runtime -sh&& scram b -j 8
triggerDoctor commented 9 months ago

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found no issues with the code checks! Info Value
return code 0
command evalscramv1 runtime -sh&& scram b -k -j 8 code-checks && scram b -k -j 8 code-checks
I found no issues with the headers! Info Value
return code 0
command evalscramv1 runtime -sh&& scram b -k -j 8 check-headers
triggerDoctor commented 9 months ago

Hello, I'm triggerDoctor. @aloeliger is testing this script for L1T offline software validation.

I found 1 files that did not meet formatting requirements:

Please run scram b code-format to auto-apply code formatting Info Value
return code 0
command evalscramv1 runtime -sh&& scram b -k -j 8 code-format-all
aloeliger commented 9 months ago

@EmyrClement

I also have a related question on how to handle the use of this customisation, which the GCT calo jets/taus require. For the upcoming reviews, we could include the customisation in the standard cmsDriver command that's used? Or should we make this change explicit in the L1 sequence so you can't forget to apply it?

The question is how permanent this "customization" is. If this is something that we always need to run, then it should of course be explicitly in the L1 sequence. But this is complicated by...

The other complication is that the NN Calo Taus may require the original HGCal towers. They are updating their training to adapt to use the same towers as the GCT calo taus, but we may need to produce two collections of HGCal towers, one with and one without the modifications introduced by the customisation. I think in this case we would need to have two sequences producing the two tower collections, rather than a customisation function that switches between the two.

My preferences, in order, to handle this would be:

  1. Having NN Calo Tau stuff ready before this becomes an issue and making the new stuff uniform.
  2. Provide a module to adapt new towers to NN Calo Tau input, and if we run a customization for new stuff (which is my understanding of this customization function?), then we insert it into the sequence
  3. If the customization for new towers is applied then have it attempt to disable NN Calo Taus.
  4. Disabling NN Calo Taus entirely until they are ready.

I would really like to avoid the jury rig and duct-tape job of running tons of customizations for current and deprecated inputs rather than having a uniform set. That's likely to start introducing maintenance issues into the code.

EmyrClement commented 9 months ago

Tentatively 1) may be done by the end of next week. If so then we can go ahead and make the config changes introduced by the customization "permanent" in the L1 sequence.

In the longer term, both the default and customised HGCal towers will be superseded, at which point we can go back to using the default towers provided to us by HGCal.

EmyrClement commented 9 months ago

@aloeliger could we merge this PR now just so the customisation is fixed in the recipe (useful for my studies)? I can follow up with another PR with the final usage of the customisation in a weeks time. No worries if not.

aloeliger commented 9 months ago

@EmyrClement Yeah, we're trying to integrate all changes before march, so we're no longer worrying about whether or not things are final. I can go ahead and merge this.

aloeliger commented 9 months ago

@EmyrClement If there is not already a PR to CMSSW for this, it is going to be on you to remember this is here or it is going to end up lost. In fact, it may be to your benefit to open a draft PR already.

EmyrClement commented 9 months ago

PR to master opened : https://github.com/cms-sw/cmssw/pull/44066