Skip to content

[HGCAL] Fix cell types in indexer#48155

Merged
cmsbuild merged 1 commit intocms-sw:masterfrom
CMS-HGCAL:dev/hackathon_celltype
May 31, 2025
Merged

[HGCAL] Fix cell types in indexer#48155
cmsbuild merged 1 commit intocms-sw:masterfrom
CMS-HGCAL:dev/hackathon_celltype

Conversation

@IzaakWN
Copy link
Copy Markdown
Contributor

@IzaakWN IzaakWN commented May 23, 2025

Use more meaningfull enums from HGCSiliconDetId and HGCScintillatorDetId for cell types (instead of magic numbers).

(Cell type was not yet used by the HGCal Raw-Data DPG, but we'll need it soon to correctly map calibrations of RecHit's energy.)

@cmsbuild
Copy link
Copy Markdown
Contributor

cmsbuild commented May 23, 2025

cms-bot internal usage

@cmsbuild
Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48155/44929

@cmsbuild
Copy link
Copy Markdown
Contributor

A new Pull Request was created by @IzaakWN for master.

It involves the following packages:

  • CondFormats/HGCalObjects (db, upgrade, alca)
  • Geometry/HGCalMapping (upgrade, geometry)

@Dr15Jones, @Moanwar, @atpathak, @bsunanda, @civanch, @cmsbuild, @francescobrivio, @kpedro88, @makortel, @mdhildreth, @perrotta, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks.
@JanChyczynski, @PonIlya, @bsunanda, @fabiocos, @martinamalberti, @mmusich, @rsreds, @seemasharmafnal, @tocheng, @yuanchao this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

@pfs
Copy link
Copy Markdown
Contributor

pfs commented May 23, 2025

assign hgcal-dpg

@pfs
Copy link
Copy Markdown
Contributor

pfs commented May 23, 2025

please test

@cmsbuild
Copy link
Copy Markdown
Contributor

New categories assigned: hgcal-dpg

@cseez,@felicepantaleo,@pfs,@rovere you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Copy Markdown
Contributor

-1

Failed Tests: Build
Size: This PR adds an extra 28KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-09dd72/46342/summary.html
COMMIT: 3bd2dfc
CMSSW: CMSSW_15_1_X_2025-05-23-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/48155/46342/install.sh to create a dev area with all the needed externals and cmssw changes.

Build

I found compilation error when building:

>> Compiling alpaka/cuda edm plugin src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitConfigurationESProducer.cc
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin/c++ -c -DCMS_MICRO_ARCH='x86-64-v3' -DGNU_GCC -D_GNU_SOURCE -DTBB_USE_GLIBCXX_VERSION=120301 -DTBB_SUPPRESS_DEPRECATED_MESSAGES -DTBB_PREVIEW_RESUMABLE_TASKS=1 -DTBB_PREVIEW_TASK_GROUP_EXTENSIONS=1 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -DBOOST_MATH_DISABLE_STD_FPCLASSIFY -DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DBOOST_MPL_IGNORE_PARENTHESES_WARNING -DDD4HEP_USE_GEANT4_UNITS=1 -DCMSSW_GIT_HASH='CMSSW_15_1_X_2025-05-23-1100' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_15_1_X_2025-05-23-1100' -Isrc -Ipoison -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_15_1_X_2025-05-23-1100/src -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/dd4hep/v01-29-00-a6251f4ae4aef60eab61c637c5fb989d/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/alpaka/1.2.0-9ea5b7b50bc68454be5c438069f1aa33/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/pcre/8.43-2d141998cfe5424b8f7aff48035cc2da/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/boost/1.80.0-083047018155d4c00e9977f5692490b3/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/bz2lib/1.0.6-d065ccd79984efc6d4660f410e4c81de/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/clhep/2.4.7.1-d3a3e353d370e701238f7949a0d7909f/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/gsl/2.6-f7574c606b0ce57ff601d3ca9534cd01/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/hls/2025.05-4da036171b10090d7d21e15c49bb27a1/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/libuuid/2.34-27ce4c3579b5b1de2808ea9c4cd8ed29/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/lcg/root/6.32.13-b42babf347314b02960f7ec363565e45/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/tbb/v2022.0.0-fe4e5fc27f2e8eae5da4b66ede97f22b/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/cms/vdt/0.4.3-f008bd09702a81e8a062bbef698ed3af/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/xerces-c/3.1.3-c7b88eaa36d0408120f3c29826a04bf6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/xz/5.2.5-6f3f49b07db84e10c9be594a1176c114/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/zlib/1.2.13-d217cdbdd8d586e845e05946de2796be/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/cuda/12.9.0-cd4613335deff6e5576167f7dbef4cb0/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include/eigen3 -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/fmt/10.2.1-e35fd1db5eb3abc8ac0452e8ee427196/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/json/3.11.3-fe8b14080d05d6ff46ded7c91e3977bc/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/md5/1.0.0-5b594b264e04ae51e893b1d69a797ec6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/OpenBLAS/0.3.27-70a9dd2c9f309171934f13e3003b0540/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/tinyxml2/6.2.0-a0ad3950415fa3138d99b7da42eb4c9f/include -O3 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++20 -ftree-vectorize -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -Wno-error=array-bounds -Warray-bounds -fuse-ld=bfd -march=x86-64-v3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wextra -Wpessimizing-move -Wclass-memaccess -Wno-cast-function-type -Wno-unused-but-set-parameter -Wno-ignored-qualifiers -Wno-unused-parameter -Wunused -Wparentheses -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=unused-label -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Wnon-virtual-dtor -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -DEIGEN_DONT_PARALLELIZE -DEIGEN_MAX_ALIGN_BYTES=64 -Wno-error=unused-variable -DALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128 -DALPAKA_DISABLE_VENDOR_RNG -DALPAKA_ACC_GPU_CUDA_ENABLED -DALPAKA_ACC_GPU_CUDA_ONLY_MODE -DALPAKA_HOST_ONLY -DBOOST_DISABLE_ASSERTS -flto=auto -fipa-icf -flto-odr-type-merging -fno-fat-lto-objects -Wodr -fPIC -MMD -MF tmp/el8_amd64_gcc12/src/RecoLocalCalo/HGCalRecAlgos/plugins/RecoLocalCaloHGCalRecAlgosPluginsPortableCudaAsync/alpaka/HGCalRecHitConfigurationESProducer.cc.d src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitConfigurationESProducer.cc -o tmp/el8_amd64_gcc12/src/RecoLocalCalo/HGCalRecAlgos/plugins/RecoLocalCaloHGCalRecAlgosPluginsPortableCudaAsync/alpaka/HGCalRecHitConfigurationESProducer.cc.o
>> Compiling alpaka/cuda edm plugin src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitProducers.cc
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/bin/c++ -c -DCMS_MICRO_ARCH='x86-64-v3' -DGNU_GCC -D_GNU_SOURCE -DTBB_USE_GLIBCXX_VERSION=120301 -DTBB_SUPPRESS_DEPRECATED_MESSAGES -DTBB_PREVIEW_RESUMABLE_TASKS=1 -DTBB_PREVIEW_TASK_GROUP_EXTENSIONS=1 -DBOOST_SPIRIT_THREADSAFE -DPHOENIX_THREADSAFE -DBOOST_MATH_DISABLE_STD_FPCLASSIFY -DBOOST_UUID_RANDOM_PROVIDER_FORCE_POSIX -DBOOST_MPL_IGNORE_PARENTHESES_WARNING -DDD4HEP_USE_GEANT4_UNITS=1 -DCMSSW_GIT_HASH='CMSSW_15_1_X_2025-05-23-1100' -DPROJECT_NAME='CMSSW' -DPROJECT_VERSION='CMSSW_15_1_X_2025-05-23-1100' -Isrc -Ipoison -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/cms/cmssw-patch/CMSSW_15_1_X_2025-05-23-1100/src -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/dd4hep/v01-29-00-a6251f4ae4aef60eab61c637c5fb989d/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/alpaka/1.2.0-9ea5b7b50bc68454be5c438069f1aa33/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/pcre/8.43-2d141998cfe5424b8f7aff48035cc2da/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/boost/1.80.0-083047018155d4c00e9977f5692490b3/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/bz2lib/1.0.6-d065ccd79984efc6d4660f410e4c81de/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/clhep/2.4.7.1-d3a3e353d370e701238f7949a0d7909f/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/gsl/2.6-f7574c606b0ce57ff601d3ca9534cd01/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/hls/2025.05-4da036171b10090d7d21e15c49bb27a1/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/libuuid/2.34-27ce4c3579b5b1de2808ea9c4cd8ed29/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/lcg/root/6.32.13-b42babf347314b02960f7ec363565e45/include -isystem/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/tbb/v2022.0.0-fe4e5fc27f2e8eae5da4b66ede97f22b/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/cms/vdt/0.4.3-f008bd09702a81e8a062bbef698ed3af/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/xerces-c/3.1.3-c7b88eaa36d0408120f3c29826a04bf6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/xz/5.2.5-6f3f49b07db84e10c9be594a1176c114/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/zlib/1.2.13-d217cdbdd8d586e845e05946de2796be/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/cuda/12.9.0-cd4613335deff6e5576167f7dbef4cb0/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/eigen/3bb6a48d8c171cf20b5f8e48bfb4e424fbd4f79e-5d91c922e771c0dc4f6bc00f61f3e2c5/include/eigen3 -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/fmt/10.2.1-e35fd1db5eb3abc8ac0452e8ee427196/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/json/3.11.3-fe8b14080d05d6ff46ded7c91e3977bc/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/md5/1.0.0-5b594b264e04ae51e893b1d69a797ec6/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/OpenBLAS/0.3.27-70a9dd2c9f309171934f13e3003b0540/include -I/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02890/el8_amd64_gcc12/external/tinyxml2/6.2.0-a0ad3950415fa3138d99b7da42eb4c9f/include -O3 -pthread -pipe -Werror=main -Werror=pointer-arith -Werror=overlength-strings -Wno-vla -Werror=overflow -std=c++20 -ftree-vectorize -Werror=array-bounds -Werror=format-contains-nul -Werror=type-limits -fvisibility-inlines-hidden -fno-math-errno --param vect-max-version-for-alias-checks=50 -Xassembler --compress-debug-sections -Wno-error=array-bounds -Warray-bounds -fuse-ld=bfd -march=x86-64-v3 -felide-constructors -fmessage-length=0 -Wall -Wno-non-template-friend -Wno-long-long -Wreturn-type -Wextra -Wpessimizing-move -Wclass-memaccess -Wno-cast-function-type -Wno-unused-but-set-parameter -Wno-ignored-qualifiers -Wno-unused-parameter -Wunused -Wparentheses -Werror=return-type -Werror=missing-braces -Werror=unused-value -Werror=unused-label -Werror=address -Werror=format -Werror=sign-compare -Werror=write-strings -Werror=delete-non-virtual-dtor -Werror=strict-aliasing -Werror=narrowing -Werror=unused-but-set-variable -Werror=reorder -Werror=unused-variable -Werror=conversion-null -Werror=return-local-addr -Wnon-virtual-dtor -Werror=switch -fdiagnostics-show-option -Wno-unused-local-typedefs -Wno-attributes -Wno-psabi -DEIGEN_DONT_PARALLELIZE -DEIGEN_MAX_ALIGN_BYTES=64 -Wno-error=unused-variable -DALPAKA_DEFAULT_HOST_MEMORY_ALIGNMENT=128 -DALPAKA_DISABLE_VENDOR_RNG -DALPAKA_ACC_GPU_CUDA_ENABLED -DALPAKA_ACC_GPU_CUDA_ONLY_MODE -DALPAKA_HOST_ONLY -DBOOST_DISABLE_ASSERTS -flto=auto -fipa-icf -flto-odr-type-merging -fno-fat-lto-objects -Wodr -fPIC -MMD -MF tmp/el8_amd64_gcc12/src/RecoLocalCalo/HGCalRecAlgos/plugins/RecoLocalCaloHGCalRecAlgosPluginsPortableCudaAsync/alpaka/HGCalRecHitProducers.cc.d src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitProducers.cc -o tmp/el8_amd64_gcc12/src/RecoLocalCalo/HGCalRecAlgos/plugins/RecoLocalCaloHGCalRecAlgosPluginsPortableCudaAsync/alpaka/HGCalRecHitProducers.cc.o
src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitCalibrationESProducer.cc: In member function 'std::optional > > alpaka_cuda_async::hgcalrechit::HGCalCalibrationESProducer::produce(const HGCalModuleConfigurationRcd&)':
src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitCalibrationESProducer.cc:109:18: error: 'LogWarning' is not a member of 'edm'
  109 |             edm::LogWarning("HGCalCalibrationESProducer")
      |                  ^~~~~~~~~~
src/RecoLocalCalo/HGCalRecAlgos/plugins/alpaka/HGCalRecHitCalibrationESProducer.cc:118:20: error: 'LogWarning' is not a member of 'edm'
  118 |               edm::LogWarning("HGCalCalibrationESProducer")
      |                    ^~~~~~~~~~


@cmsbuild
Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48155/44930

@cmsbuild
Copy link
Copy Markdown
Contributor

@cmsbuild
Copy link
Copy Markdown
Contributor

Pull request #48155 was updated. @Moanwar, @atpathak, @cmsbuild, @cseez, @felicepantaleo, @francescobrivio, @perrotta, @pfs, @rovere, @srimanob, @subirsarkar can you please check and sign again.

@pfs
Copy link
Copy Markdown
Contributor

pfs commented May 27, 2025

please test

@cmsbuild
Copy link
Copy Markdown
Contributor

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-09dd72/46371/summary.html
COMMIT: 41c7250
CMSSW: CMSSW_15_1_X_2025-05-26-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/48155/46371/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 6 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 4038193
  • DQMHistoTests: Total failures: 22
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4038151
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 215 log files, 184 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@cmsbuild
Copy link
Copy Markdown
Contributor

Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc Outdated
Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc
Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc
… -> getCellType; use int8 for celltype

and squashed commit of the following:

commit cba58c3
Author: IzaakWN <iwn_@hotmail.com>
Date:   Mon May 26 19:22:00 2025 +0200

    fix typo: get second character in typecode for isHD

commit 636eaef
Merge: d1ba95e 2d878df
Author: Izaak <iwn_@hotmail.com>
Date:   Sun May 25 20:40:08 2025 +0200

    Merge branch 'master' into dev/hackathon_celltype

commit d1ba95e
Author: IzaakWN <iwn_@hotmail.com>
Date:   Fri May 23 11:20:47 2025 +0200

    convert module type codes to DetID cell types; rename convertTypeCode -> getCellType
@cmsbuild
Copy link
Copy Markdown
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-48155/44970

@cmsbuild
Copy link
Copy Markdown
Contributor

Pull request #48155 was updated. @Moanwar, @atpathak, @cmsbuild, @cseez, @felicepantaleo, @francescobrivio, @jfernan2, @mandrenguyen, @perrotta, @pfs, @rovere, @srimanob, @subirsarkar can you please check and sign again.

Copy link
Copy Markdown
Contributor Author

@IzaakWN IzaakWN left a comment

Choose a reason for hiding this comment

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

Thank you for the careful review, @perrotta. As you can see, I fixed the flagged issues, and changed the celltype to int8_t everywhere. The commits are squashed into one.

Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc
Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc
Comment thread RecoLocalCalo/HGCalRecAlgos/src/HGCalESProducerTools.cc
Comment thread CondFormats/HGCalObjects/src/HGCalMappingModuleIndexer.cc
@perrotta
Copy link
Copy Markdown
Contributor

(Cell type was not yet used by the HGCal Raw-Data DPG, but we'll need it soon to correctly map calibrations of RecHit's energy.)

Evidently this code was never used and not even tested before. Does a workflow exist to test it? Can it be enabled by any switch?

@IzaakWN
Copy link
Copy Markdown
Contributor Author

IzaakWN commented May 29, 2025

Evidently this code was never used

The value of celltype itself has not been used yet. This PR only changes the value of celltype to a meaningful enum that is common with DetID.

and not even tested before.

However, the ModuleIndexer and the mapping SoAs that contains the cell type have been used extensively for HGCal commissioning in the lab and during test beams in the past couple of years. We used it for processing (RAW → FEDRaw → DIGI → RecHit), analyzing, calibrating, and monitoring the data. We did not put in release part of the workflow that is specific to test beams.

I tested this PR with our workflow for commissioning, as well as the standalone test scripts that are in release, e.g. RecoLocalCalo/HGCalRecAlgos/test/testHGCalRecHitESProducers_cfg.py or Geometry/HGCalMapping/test/testMappingModuleIndexer_cfg.py.

Does a workflow exist to test it? Can it be enabled by any switch?

We are close to finishing full workflows with simulated ttbar data, but that is outside the scope of this PR.

@perrotta
Copy link
Copy Markdown
Contributor

I tested this PR with our workflow for commissioning, as well as the standalone test scripts that are in release, e.g. RecoLocalCalo/HGCalRecAlgos/test/testHGCalRecHitESProducers_cfg.py or Geometry/HGCalMapping/test/testMappingModuleIndexer_cfg.py.

And how do those tests miss the fact that a variable which is supposed to take several integer values, was rendered instead (also in the original code in the baseline release) as a bool, i.e. only 0 and 1 once the type was casted back to int, see e.g. https://github.com/cms-sw/cmssw/pull/48155/files#diff-2b2674086626df823c93b363be7d0fd6ebcb66d7ae0a37b1c0a450526302d642R66 ?

@IzaakWN
Copy link
Copy Markdown
Contributor Author

IzaakWN commented May 29, 2025

I tested this PR with our workflow for commissioning, as well as the standalone test scripts that are in release, e.g. RecoLocalCalo/HGCalRecAlgos/test/testHGCalRecHitESProducers_cfg.py or Geometry/HGCalMapping/test/testMappingModuleIndexer_cfg.py.

And how do those tests miss the fact that a variable which is supposed to take several integer values, was rendered instead (also in the original code in the baseline release) as a bool, i.e. only 0 and 1 once the type was casted back to int, see e.g. https://github.com/cms-sw/cmssw/pull/48155/files#diff-2b2674086626df823c93b363be7d0fd6ebcb66d7ae0a37b1c0a450526302d642R66 ?

Because I was sloppy and did not look at the value of the celltype.

@perrotta
Copy link
Copy Markdown
Contributor

please test

@perrotta
Copy link
Copy Markdown
Contributor

Does a workflow exist to test it? Can it be enabled by any switch?

We are close to finishing full workflows with simulated ttbar data, but that is outside the scope of this PR.

Then, also given what is written in a previous comment, it is clear that this code was never tested so far. This is an original sin of the code, which only surfaced here.
When the PR tests will complete I can sign this update for db and alca (but then there are other categories whose signature is also needed, and they could have a different opinion on it). Still, untill the full workflow intended to test this code that you are mentioning is not submitted (in another PR) all this will remain as a dead and unattended branch in cmssw, which is something that should be avoided.

@cmsbuild
Copy link
Copy Markdown
Contributor

+1

Size: This PR adds an extra 16KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-09dd72/46451/summary.html
COMMIT: d308287
CMSSW: CMSSW_15_1_X_2025-05-29-2300/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/48155/46451/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

  • You potentially removed 1 lines from the logs
  • Reco comparison results: 1 differences found in the comparisons
  • DQMHistoTests: Total files compared: 50
  • DQMHistoTests: Total histograms compared: 4041063
  • DQMHistoTests: Total failures: 22
  • DQMHistoTests: Total nulls: 0
  • DQMHistoTests: Total successes: 4041021
  • DQMHistoTests: Total skipped: 20
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: 0.0 KiB( 49 files compared)
  • Checked 215 log files, 184 edm output root files, 50 DQM output files
  • TriggerResults: no differences found

@perrotta
Copy link
Copy Markdown
Contributor

+1

  • With the caveat (better described here above) that no workflow exists yet to test the features updated in this PR: in fact a bug, already present in the original code and then fixed in this PR, was found only based on a visual inspection of the code

@jfernan2
Copy link
Copy Markdown
Contributor

+1

@Moanwar
Copy link
Copy Markdown
Contributor

Moanwar commented May 30, 2025

+Upgrade

@pfs
Copy link
Copy Markdown
Contributor

pfs commented May 31, 2025

+1

@cmsbuild
Copy link
Copy Markdown
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy (and backports should be raised in the release meeting by the corresponding L2)

@mandrenguyen
Copy link
Copy Markdown
Contributor

+1

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.

8 participants