search

Mu2e / Offline

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

Introduce SurfaceStep #1300

Closed brownd1978 closed 2 months ago

brownd1978 commented 3 months ago

SurfaceStep is a new MC truth class that functions like StrawGasStep, CaloShowerStep, etc, but for passive materials and virtual detectors. It uses the SurfaceId class to define the surfaces, and summarizes the information contained in contiguous StepPointMC objects. This greatly reduced the payload and renders the output information more useful for physics studies, and insulates the results from the details of the G4 stepping parameters. This PR introduces the class itself plus producer, diag module, printer, and default production configuration.

Note that SurfaceId was moved to DataProducts (from RecoDataProducts and KinKalGeom) to allow this to work. It's fine, as the surface concept isn't specific to reco or mc.

FNALbuild commented 3 months ago

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

which require these tests: build.

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

:hourglass: The following tests have been triggered for 680a6ef27db80a51e5b75c6a412cff77e94007ef: build (Build queue has 1 jobs)

About FNALbuild. Code review on Mu2e/Offline.

FNALbuild commented 3 months ago

:sunny: The build tests passed at 680a6ef27db80a51e5b75c6a412cff77e94007ef.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 680a6ef27db80a51e5b75c6a412cff77e94007ef at cecf5be9ef6d631941dda405ade22c77a16b04fd
build (prof) :white_check_mark: Log file. Build time: 08 min 24 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 :large_orange_diamond: TODO (21) FIXME (2) in 15 files
clang-tidy :large_orange_diamond: 0 errors 580 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 680a6ef27db80a51e5b75c6a412cff77e94007ef after being merged into the base branch at cecf5be9ef6d631941dda405ade22c77a16b04fd.

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.

brownd1978 commented 2 months ago
  • "surface" would be more clear as "material" or "volume" Surface is explicit in the downstream use cases, particularly reconstruction. 'volume' or 'material' are less accurate.
  • split SurfaceId from enum These classes don't make sense as independent objects, just like the detail of the enum class it is clearer to keep them in 1 file
  • SurfaceId::index could be named more clearly and commented I added some comments. The name must be general.
  • fix include guards in SurfaceId done
  • print looks fine

Note too SurfaceId pre-exists this PR by over a year, this PR simply moves it.

kutschke commented 2 months ago

@sdifalco a gentle reminder that this PR is on your to-review list.

kutschke commented 2 months ago

@FNALbuild run build test

FNALbuild commented 2 months ago

:hourglass: The following tests have been triggered for 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c: build (Build queue has 1 jobs)

FNALbuild commented 2 months ago

:umbrella: The build tests failed for 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c.

Test Result Details
test with :white_check_mark: Command did not list any other PRs to include
merge :white_check_mark: Merged 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c at 616f60bcd027766d9b4b7f6641bb9d16a35b1e5f
build (prof) :white_check_mark: Log file. Build time: 04 min 10 sec
ceSimReco :x: Log file. Return Code 90.
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 :x: Log file. Return Code 90.
cosmicOffSpill :x: Log file. Return Code 90.
ceSteps :x: Log file. Return Code 90.
ceDigi :x: Log file. Return Code 90.
muDauSteps :x: Log file. Return Code 90.
ceMix :x: Log file. Return Code 90.
rootOverlaps :white_check_mark: Log file.
g4surfaceCheck :white_check_mark: Log file.
FIXME, TODO :large_orange_diamond: TODO (21) FIXME (2) in 15 files
clang-tidy :large_orange_diamond: 0 errors 614 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c after being merged into the base branch at 616f60bcd027766d9b4b7f6641bb9d16a35b1e5f.

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.

kutschke commented 2 months ago

Needs to test with a Production PR

@FNALbuild run build test with Mu2e/Production#329

FNALbuild commented 2 months ago

:hourglass: The following tests have been triggered for 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c: build (Build queue has 1 jobs)

FNALbuild commented 2 months ago

:sunny: The build tests passed at 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c.

Test Result Details
test with :white_check_mark: Included Mu2e/Production#329 @ 5287648aedc1c528dafe514365f75b6a1a4f8e83 by merge
merge :white_check_mark: Merged 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c at 616f60bcd027766d9b4b7f6641bb9d16a35b1e5f
build (prof) :white_check_mark: Log file. Build time: 08 min 22 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 :large_orange_diamond: TODO (21) FIXME (2) in 15 files
clang-tidy :large_orange_diamond: 0 errors 614 warnings
whitespace check :white_check_mark: no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 491ff6ca4dcc689e64e6da9fa96b1b5e2d877e2c after being merged into the base branch at 616f60bcd027766d9b4b7f6641bb9d16a35b1e5f.

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.

brownd1978 commented 2 months ago

AFAIK I've addressed all comments and have no further plans to work on this. Stefano was added as reviewer just to keep him in the loop.

FNALbuild commented 2 months ago

:memo: The HEAD of main has changed to 183a498223a7632d51b8f342f4d49718073e43d3. Tests are now out of date.

sdifalco commented 2 months ago

@sdifalco a gentle reminder that this PR is on your to-review list.

Sorry but I am been (and still am) very busy with funding requests to INFN in these days. Thanks for taking me in the loop. I will approve what you have already commented. Thanks Ed for the nice job!

kutschke commented 2 months ago

AFAIK I've addressed all comments and have no further plans to work on this. Stefano was added as reviewer just to keep him in the loop.

In that case it's ready to merge. Do you expect this to have impact on the nightly validation? The STM PR that I merged earlier does modify the G4 geometry but only in the STM region.

kutschke commented 2 months ago

I will go ahead and merge now under the assumption that if there are issues to sort out with the nightly, we can do it tomoorow