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.

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

Rename reduceRange to reducePhiRange #47154

Merged

Conversation

VourMa
Copy link
Contributor

@VourMa VourMa commented Jan 21, 2025

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: #47033 (comment).

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
Copy link
Contributor

cmsbuild commented Jan 21, 2025

cms-bot internal usage

@cmsbuild
Copy link
Contributor

+code-checks

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

  • There are other open Pull requests which might conflict with changes you have proposed:

@cmsbuild
Copy link
Contributor

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

It involves the following packages:

  • DQM/TrackingMonitor (dqm)
  • DataFormats/GeometryVector (simulation)
  • DataFormats/L1TParticleFlow (l1, upgrade)
  • DataFormats/Math (reconstruction)
  • HeterogeneousCore/AlpakaMath (heterogeneous)
  • L1Trigger/L1THGCal (l1, upgrade)
  • L1Trigger/Phase2L1ParticleFlow (l1, upgrade)
  • L1Trigger/TrackFindingTracklet (l1)
  • RecoMuon/L2MuonSeedGenerator (reconstruction, hlt)

@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
Copy link
Contributor

fwyzard commented Jan 21, 2025

+heterogeneous

Thanks @VourMa

@mmusich
Copy link
Contributor

mmusich commented Jan 21, 2025

@cmsbuild, please test

@mmusich
Copy link
Contributor

mmusich commented Jan 21, 2025

@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
Copy link
Contributor

smuzaffar commented Jan 21, 2025

@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 cms-sw/cms-bot#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
Copy link
Contributor

mmusich commented Jan 21, 2025

+hlt

  • technical

@jfernan2
Copy link
Contributor

+1

@Moanwar
Copy link
Contributor

Moanwar commented Jan 22, 2025

+Upgrade

@VourMa
Copy link
Contributor Author

VourMa commented Jan 22, 2025

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
Copy link
Contributor

civanch commented Jan 22, 2025

+1

@skinnari
Copy link
Contributor

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

@antoniovagnerini
Copy link

+dqm

@slava77
Copy link
Contributor

slava77 commented Jan 24, 2025

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

@aloeliger
Copy link
Contributor

+l1

@cmsbuild
Copy link
Contributor

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
Copy link
Contributor

+1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment