search

Mu2e / Offline

Offline software for the Mu2e experiment
Apache License 2.0
8 stars 79 forks source link

Bug fixed in const variable definition #1292

Closed sdifalco closed 2 weeks ago

sdifalco commented 2 weeks ago

Constant variables used to set the interval of RandomUnitSphere required a static prefix. Some more histograms added for debug purposes.

FNALbuild commented 2 weeks ago

Hi @sdifalco, You have proposed changes to files in these packages:

which require these tests: build.

@Mu2e/fnalbuild-users, @Mu2e/write have access to CI actions on main.

:hourglass: The following tests have been triggered for 5dc4f81f0fb263d32cfbfe93407e2a4b2aaac426: build (Build queue has 5 jobs)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild commented 2 weeks ago

:sunny: The build tests passed at 5dc4f81f0fb263d32cfbfe93407e2a4b2aaac426.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 5dc4f81f0fb263d32cfbfe93407e2a4b2aaac426 at 684bac4f22a1fa29f6bfd661300432e492dccbdd
build (prof) :white_check_mark: Log file. Build time: 04 min 11 sec
ceSimReco :white_check_mark: Log file.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :white_check_mark: TODO (0) FIXME (0) in 1 files
clang-tidy :large_orange_diamond: 0 errors 109 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 5dc4f81f0fb263d32cfbfe93407e2a4b2aaac426 after being merged into the base branch at 684bac4f22a1fa29f6bfd661300432e492dccbdd.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

gaponenko commented 2 weeks ago

Or use the std::numbers way? https://en.cppreference.com/w/cpp/numeric/constants

Andrei

From: Rob Kutschke @.***> Sent: Tuesday, June 25, 2024 9:40 AM To: Mu2e/Offline Cc: Andrei Gaponenko; Team mention Subject: Re: [Mu2e/Offline] Bug fixed in const variable definition (PR #1292)

[EXTERNAL] – This message is from an external sender

@kutschke requested changes on this pull request.

One request for a small change

In EventGenerator/src/RPCGun_module.cchttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1292-23discussion-5Fr1652963680&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=VaK73zJDw4asXNvqjZzItd1CF8lIfRFcbdq1Wza2KEs&e=:

@@ -83,16 +83,22 @@ namespace mu2e { RandomUnitSphere randomUnitSphere; CLHEP::RandGeneral randSpectrum;

Please use 2.*M_PI .

— Reply to this email directly, view it on GitHubhttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1292-23pullrequestreview-2D2138906076&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=D4borDnI0A-aGBrxWTv8iSyFlDF_Hh45_H3YKf3Otww&e=, or unsubscribehttps://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAXVCGRAHZC6I7R4EIJ3LDTZJF6M7AVCNFSM6AAAAABJ327XX6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZYHEYDMMBXGY&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=VKz1rriVrYVUc3MR_a5VRX8tU6q_3IEwzg1oaB6Fhb0&e=. You are receiving this because you are on a team that was mentioned.Message ID: @.***>

brownd1978 commented 2 weeks ago

I think leaving these out of this function completely is the right answer: the default constructor sets the values to correct values, no need to override.

On Tue, Jun 25, 2024 at 8:58 AM Andrei Gaponenko @.***> wrote:

Or use the std::numbers way? https://en.cppreference.com/w/cpp/numeric/constants

Andrei

From: Rob Kutschke @.***> Sent: Tuesday, June 25, 2024 9:40 AM To: Mu2e/Offline Cc: Andrei Gaponenko; Team mention Subject: Re: [Mu2e/Offline] Bug fixed in const variable definition (PR

1292)

[EXTERNAL] – This message is from an external sender

@kutschke requested changes on this pull request.

One request for a small change

In EventGenerator/src/RPCGun_module.cc< https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1292-23discussion-5Fr1652963680&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=VaK73zJDw4asXNvqjZzItd1CF8lIfRFcbdq1Wza2KEs&e=>:

@@ -83,16 +83,22 @@ namespace mu2e { RandomUnitSphere randomUnitSphere; CLHEP::RandGeneral randSpectrum;

  • const double czmin_ = -1;
  • const double czmax_ = 1;
  • const double phimin_ = 0;
  • const double phimax_ = CLHEP::twopi;
  • static constexpr double czmin_ = -1;
  • static constexpr double czmax_ = 1;
  • static constexpr double phimin_ = 0;
  • static constexpr double phimax_ = CLHEP::twopi;

Please use 2.*M_PI .

— Reply to this email directly, view it on GitHub< https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_Mu2e_Offline_pull_1292-23pullrequestreview-2D2138906076&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=D4borDnI0A-aGBrxWTv8iSyFlDF_Hh45_H3YKf3Otww&e=>, or unsubscribe< https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_notifications_unsubscribe-2Dauth_AAXVCGRAHZC6I7R4EIJ3LDTZJF6M7AVCNFSM6AAAAABJ327XX6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZYHEYDMMBXGY&d=DwMFaQ&c=gRgGjJ3BkIsb5y6s49QqsA&r=O47fc5vzDTR2V_gla4Ub0Q&m=UjEzACLduoY_NeqghSgH9vtdRtvIe6VblFJ5v7FyGIOAzETt9kgHe3YNvdPcWwlP&s=VKz1rriVrYVUc3MR_a5VRX8tU6q_3IEwzg1oaB6Fhb0&e=>.

You are receiving this because you are on a team that was mentioned.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1292#issuecomment-2189341004, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH57YIIMF6X4OBKILEI7LZJGHT5AVCNFSM6AAAAABJ327XX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBZGM2DCMBQGQ . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

kutschke commented 2 weeks ago

I think leaving these out of this function completely is the right answer: the default constructor sets the values to correct values, no need to override.

The design is that RandomUnitSphere defaults to full 4 pi but has c'tor options to generate over a restricted range. It looks like the authors of this module are not interested in the functionality to generate over a restricted range. In that case, I agree with Dave, let the range arguments take their default values.

@sdifalco is there really no use case to generate on a subset of the unit sphere?

sdifalco commented 2 weeks ago

Good catch. So because of the initialization order those limits were set to 'random' (uninitializd) values when used to build the sphere. I wonder what effect this had on RPC in SU2020.

The other solution is to not provide any values, since those are the default values. It is natural to expect a sphere to be a sphere, not a partial sphere.

I have looked in SU2020 repository and there RPCGunmodule.cc has the limits of parameters not as constants: , czmin (pset.get("czmin" )) , czmax (pset.get("czmax" )) , phimin (pset.get("phimin")) , phimax (pset.get("phimax")) ... if (phimax > CLHEP::twopi) phimax_ = CLHEP::twopi;

The default values were set in EventGenerator/fcl/prolog.fcl: RPCGun : { module_type : RPCGun verbosityLevel : 0 doHistograms : false tmin : -1. czmin : -1. czmax : 1. phimin : 0. phimax : 10.

The same approach was used for delayed RPC coming from antiprotons.

So I don't see issues in SU2020 simulations.

FNALbuild commented 2 weeks ago

:memo: The HEAD of main has changed to 825dc593c6dbdb4054a1b746f46b61a538a7075e. Tests are now out of date.

sdifalco commented 2 weeks ago

I think leaving these out of this function completely is the right answer: the default constructor sets the values to correct values, no need to override.

The design is that RandomUnitSphere defaults to full 4 pi but has c'tor options to generate over a restricted range. It looks like the authors of this module are not interested in the functionality to generate over a restricted range. In that case, I agree with Dave, let the range arguments take their default values.

@sdifalco is there really no use case to generate on a subset of the unit sphere?

I must say that I am not the author of this function and that in SU2020 simulation the defaults corresponding to the whole sphere have not been changed. I agree in removing the constants and I don't see at the moment the need to generate on a fraction of sphere.

sdifalco commented 2 weeks ago

@FNALbuild run build test

FNALbuild commented 2 weeks ago

:hourglass: The following tests have been triggered for d9ec89b2149b3196e7e267dae9ea87bf40a05d6b: build (Build queue has 17 jobs)

brownd1978 commented 2 weeks ago

Thanks for removing the constants. Any update to randomunitsphere can be handled in a separate PR.

On Tue, Jun 25, 2024 at 1:22 PM Fermilab build bot account < @.***> wrote:

⌛ The following tests have been triggered for d9ec89b https://github.com/Mu2e/Offline/commit/d9ec89b2149b3196e7e267dae9ea87bf40a05d6b: build (Build queue has 17 jobs)

— Reply to this email directly, view it on GitHub https://github.com/Mu2e/Offline/pull/1292#issuecomment-2189899792, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABAH574VRAD73FEFGJ7UU7TZJHGOZAVCNFSM6AAAAABJ327XX6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBZHA4TSNZZGI . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

-- David Brown @.*** Office Phone (510) 486-7261 Lawrence Berkeley National Lab M/S 50R5008 (50-6026C) Berkeley, CA 94720

kutschke commented 2 weeks ago

Presuming the CI completes OK, I will merge.

For future reference, we now advise against putting the documentary histograms in the generator module. We advise making a companion Analyzer to make the histograms. In the early days the advice was the opposite. The reason for the change is that we never enable these histograms in production jobs and if we have them as a separate module we can add that module to the path in other jobs.

Please keep this in mid as you work on generators in the future.

FNALbuild commented 2 weeks ago

:sunny: The build tests passed at d9ec89b2149b3196e7e267dae9ea87bf40a05d6b.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged d9ec89b2149b3196e7e267dae9ea87bf40a05d6b at 825dc593c6dbdb4054a1b746f46b61a538a7075e
build (prof) :white_check_mark: Log file. Build time: 08 min 23 sec
ceSimReco :white_check_mark: Log file.
g4test_03MT :white_check_mark: Log file.
transportOnly :white_check_mark: Log file.
POT :white_check_mark: Log file.
g4study :white_check_mark: Log file.
cosmicSimReco :white_check_mark: Log file.
cosmicOffSpill :white_check_mark: Log file.
ceSteps :white_check_mark: Log file.
ceDigi :white_check_mark: Log file.
muDauSteps :white_check_mark: Log file.
ceMix :white_check_mark: Log file.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :white_check_mark: TODO (0) FIXME (0) in 1 files
clang-tidy :large_orange_diamond: 0 errors 109 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at d9ec89b2149b3196e7e267dae9ea87bf40a05d6b after being merged into the base branch at 825dc593c6dbdb4054a1b746f46b61a538a7075e.

For more information, please check the job page here. Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.