Skip to content

Conversation

@tj710
Copy link

@tj710 tj710 commented May 15, 2025

PR description:

We add new data formats for L1 Scouting, which will this year be collecting the L1 Calo Trigger 'towers', in addition to the contents of the previous years.

PR validation:

The tests are documented in /DataFormats/L1Scouting/test/TestL1ScoutingFormat.sh

If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:

We require backport to CMSSW 16_1_X for data taking purposes from start of 2026 data taking.

Before submitting your pull requests, make sure you followed this checklist:

Associated data files are in: cms-data/DataFormats-L1Scouting#5

@cmsbuild cmsbuild added this to the CMSSW_15_1_X milestone May 15, 2025
@cmsbuild
Copy link
Contributor

This PR contains too many commits (400 >= 240) and will not be processed.
Please ensure you have selected the correct target branch and consider squashing unnecessary commits.
The processing of this PR will resume once the commit count drops below the limit.

@cmsbuild
Copy link
Contributor

cmsbuild commented May 15, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/44845

ERROR: Build errors found during clang-tidy run.

src/L1TriggerScouting/OnlineProcessing/plugins/ScoutingJetProducer.cc:13:10: error: 'FWCore/Utilities/interface/Span.h' file not found [clang-diagnostic-error]
   13 | #include "FWCore/Utilities/interface/Span.h"
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Suppressed 833 warnings (833 in non-user code).
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

@cmsbuild
Copy link
Contributor

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/44855

Code check has found code style and quality issues which could be resolved by applying following patch(s)

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/47582

@cmsbuild
Copy link
Contributor

Pull request #48093 was updated. @BenjaminRS, @cmsbuild, @emeschi, @quinnanm, @smorovic can you please check and sign again.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/47583

@cmsbuild
Copy link
Contributor

Pull request #48093 was updated. @BenjaminRS, @cmsbuild, @emeschi, @quinnanm, @smorovic can you please check and sign again.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/47585

@cmsbuild
Copy link
Contributor

Pull request #48093 was updated. @BenjaminRS, @cmsbuild, @emeschi, @quinnanm, @smorovic can you please check and sign again.

Copy link
Contributor

@missirol missirol left a comment

Choose a reason for hiding this comment

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

Two more comments (hopefully, the last ones).

Thanks for changing the position of mass in the jet data format.

#define DataFormats_L1Scouting_L1ScoutingCaloTower_h

#include "DataFormats/L1Scouting/interface/OrbitCollection.h"

Copy link
Contributor

Choose a reason for hiding this comment

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

Because of int16_t, this file should now include

#include <cstdint>

Copy link
Author

Choose a reason for hiding this comment

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

Done. Do you know why the compiler does not complain about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not really. I guess it's because OrbitCollection.h (which is also included) happens to include itself cstdint.

Copy link
Author

Choose a reason for hiding this comment

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

So should I still add it to this file, or is it not needed? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Generally the best practice is for every header and source file to #include the headers that it needs rather than rely on transitive #includes. E.g. hypothetically changing OrbitCollection.h to not #include <cstdint> would result in compilation failures here.

erBits = ((ct_raw >> l1ScoutingRun3::calol2::shiftsCaloTowers::erBits) &
l1ScoutingRun3::calol2::masksCaloTowers::erBits);
miscBits = ((ct_raw >> l1ScoutingRun3::calol2::shiftsCaloTowers::miscBits) &
l1ScoutingRun3::calol2::masksCaloTowers::miscBits);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see if I get it (maybe I do not).

  • erBits is shifted by 9 and the mask is 0x000e == 1110 , so the selected bits are (10,11,12).
  • miscBits is shifted by 12 and the mask is 0x000f == 1111, so the selected bits are (12,13,14,15).

It looks like bit 12 is used for both. Is that right ? If so, intended ?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Marino

I understand it as the following:
- erBits is shifted by 9 and the mask is 0x000e == 1110 , so the selected bits are (9,10,11).

  • miscBits is shifted by 12 and the mask is 0x000f == 1111, so the selected bits are (12,13,14,15).

So there is no overlap.

Thanks a lot

Best regards

Thomas

Copy link
Contributor

@RoccoA97 RoccoA97 Jan 21, 2026

Choose a reason for hiding this comment

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

Hi @tj710. What @missirol is saying is correct and that pointed to a bug in the code

Firstly, this is the format of the tower as it is sent by the scouting readout boards

bits content
31-24 ieta (signed, int8_t)
23-16 iphi (unsigned, uint8_t)
15 Ecal fine-grain flag
14 Hcal feature flag
13 E-over-H flag
12 Denominator=0 flag
11-9 Energy ratio
0-8 Total energy (Ecal + Hcal)

The mask for the energy ratio (erBits) is currently 0x000e and the shift is 9. Now that I am looking again at this, this is not correct (it was a typo made in a commit where I have introduced the shifts) and it indeed causes what Marino is describing. The correct mask should be 0x0007 if we use 9 as shift. Can you update?

Copy link
Author

Choose a reason for hiding this comment

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

Hi Rocco, thanks for explaining.

It's now fixed.

};
} // namespace bmtf

namespace calol2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should change namespace name to calol1?

Copy link
Author

Choose a reason for hiding this comment

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

changed

namespace calol2 {
struct masksCaloTowers {
static constexpr uint32_t ET = 0x01ff;
static constexpr uint32_t erBits = 0x000e;
Copy link
Contributor

Choose a reason for hiding this comment

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

This mask should be 0x0007 (mea culpa)

Copy link
Author

Choose a reason for hiding this comment

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

fixed

};
} // namespace bmtf

namespace calol2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as before, maybe this namespace should be calol1

Copy link
Author

Choose a reason for hiding this comment

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

changed

Comment on lines 112 to 115
static constexpr uint32_t zeroFlag = 0x0001;
static constexpr uint32_t eohrFlag = 0x0002;
static constexpr uint32_t hcalFlag = 0x0004;
static constexpr uint32_t ecalFlag = 0x0008;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not as relevant as before, but all these masks should be 0x0001. It is not a big deal as we unpack all of them together in miscBits rather than one by one

Copy link
Author

Choose a reason for hiding this comment

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

fixed

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48093/47606

@cmsbuild
Copy link
Contributor

Pull request #48093 was updated. @BenjaminRS, @cmsbuild, @emeschi, @quinnanm, @smorovic can you please check and sign again.

@tj710 tj710 requested a review from RoccoA97 January 21, 2026 16:51
@missirol
Copy link
Contributor

@cms-sw/l1-l2 , do you have any further comments on this PR ?

  • It would be good to move forward, since this is related to 2026 data-taking (so, to be backported to 16_0_X).
  • It looks like #48093 (comment) is the only outstanding comment (by now it applies to only one plugin, i.e. ScoutingJetProducer). I would suggest to leave it to a follow-up PR, to avoid delaying this one (I can take care of that, if that helps).

@tj710, before this gets merged, could you please squash this PR into one (or, just a few) commits, to have a cleaner git history ? (the reviewers might ask this anyway)

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.