cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.12k stars 4.38k forks source link

Rename reduceRange to reducePhiRange #47154

Closed VourMa closed 2 weeks ago

VourMa commented 2 weeks ago

During the review of #47033 it was pointed out that the name reduceRange for the functions that restrict the φ angle is too generic and provides no connection to the fact that it is meant to be used for angles: https://github.com/cms-sw/cmssw/pull/47033#discussion_r1910650586.

This PR aims to rename the relevant function(s) to reducePhiRange to make their purpose clear. It is purely technical (no changes expected) and has been validated by making sure that the code compiles successfully.

cmsbuild commented 2 weeks ago

cms-bot internal usage

cmsbuild commented 2 weeks ago

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-47154/43384

cmsbuild commented 2 weeks ago

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

It involves the following packages:

@Martin-Grunewald, @Moanwar, @aloeliger, @antoniovagnerini, @civanch, @cmsbuild, @epalencia, @fwyzard, @jfernan2, @kpedro88, @makortel, @mandrenguyen, @mdhildreth, @mmusich, @rseidita, @srimanob, @subirsarkar can you please review it and eventually sign? Thanks. @CeliaFernandez, @Fedespring, @HuguesBrun, @Martin-Grunewald, @VinInn, @VourMa, @abbiendi, @amarini, @andrea21z, @arossi83, @bellan, @cericeci, @erikbutz, @fabiocos, @felicepantaleo, @fioriNTU, @idebruyn, @jandrea, @jbsauvan, @jhgoh, @lgray, @makortel, @missirol, @mmusich, @mtosi, @richa2710, @rociovilar, @rovere, @skinnari, @sroychow, @threus, @trocino 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

fwyzard commented 2 weeks ago

+heterogeneous

Thanks @VourMa

mmusich commented 2 weeks ago

@cmsbuild, please test

mmusich commented 2 weeks ago

@smuzaffar looks like the +heterogeneous above didn't trigger automatically a test, is it the expected behaviour? I thought before after +1-ing if it wasn't test approved it would have had triggered a testing round.

smuzaffar commented 2 weeks ago

@smuzaffar looks like the +heterogeneous above didn't trigger automatically a test, is it the expected behaviour? I thought before after +1-ing if it wasn't test approved it would have had triggered a testing round.

yes @mmusich it is expected. Auto tests were disabled here https://github.com/cms-sw/cms-bot/pull/2389 . This was done to avoid running tests specially if a category has already signed and someone pushes new code for a different category

mmusich commented 2 weeks ago

+hlt

cmsbuild commented 2 weeks ago

-1

Failed Tests: UnitTests Size: This PR adds an extra 124KB to repository Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-9c9d9a/43885/summary.html COMMIT: 3139210e7497f0286af177c9724640503f1a82c7 CMSSW: CMSSW_15_0_X_2025-01-20-2300/el8_amd64_gcc12 User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/47154/43885/install.sh to create a dev area with all the needed externals and cmssw changes.

Unit Tests

I found 2 errors in the following unit tests:

---> test EoP had ERRORS
---> test testEoPPlotting had ERRORS

Comparison Summary

Summary:

fwyzard commented 2 weeks ago

ignore tests-rejected with ib-failure

jfernan2 commented 2 weeks ago

+1

Moanwar commented 2 weeks ago

+Upgrade

VourMa commented 2 weeks ago

Any possibility to have this signed off and merged soon?

It is a simple renaming which works fine in the tests (unit test failures are unrelated), and it is needed for a follow up PR I am preparing.

civanch commented 2 weeks ago

+1

skinnari commented 2 weeks ago

FYI @tomalin @aehart (this change is to a CMSSW reco:: function, but it is used extensively in our tracklet code)

antoniovagnerini commented 2 weeks ago

+dqm

slava77 commented 2 weeks ago

@cms-sw/l1-l2 your signature is needed for this PR. Please check and perhaps comment or sign. Thank you.

aloeliger commented 2 weeks ago

+l1

cmsbuild commented 2 weeks ago

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

mandrenguyen commented 2 weeks ago

+1