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

Added postMomentum to StepPointMC #379

Merged
merged 1 commit into from
Mar 20, 2021
Merged

Added postMomentum to StepPointMC #379

merged 1 commit into from
Mar 20, 2021

Conversation

resnegfk
Copy link
Contributor

Added post step momentum to StepPointMC to enable future use when creating higher level objects

@FNALbuild
Copy link
Collaborator

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

  • UtilityModules
  • MCDataProducts
  • EventGenerator
  • Mu2eG4

which require these tests: build.

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

The following users requested to be notified about changes to these packages:
@resnegfk
⌛ The following tests have been triggered for 75a558b: build (Build queue is empty)

About FNALbuild. Code review on Mu2e/Offline.

@FNALbuild
Copy link
Collaborator

☀️ The tests passed at ref 75a558b.

Test Result Details
merge ✔️ Merged 75a558b at e9b5b8e
scons build (prof) ✔️ Log file. Build time: 14 min 33 sec
ceSimReco ✔️ Log file
transportOnly ✔️ Log file
PS ✔️ Log file
g4study ✔️ Log file
cosmicSimReco ✔️ Log file
rootOverlaps ✔️ Log file
g4surfaceCheck ✔️ Log file
g4test_03MT ✔️ Log file
FIXME, TODO count 〰️ TODO (0) FIXME (2) in 16 files
clang-tidy 〰️ 0 errors 19 warnings

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

Logfiles may also be accessed at this link.

Copy link
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Can you please remind me which use case(s) this addresses?

@resnegfk resnegfk marked this pull request as draft March 19, 2021 17:57
@resnegfk resnegfk marked this pull request as ready for review March 20, 2021 17:39
@resnegfk
Copy link
Contributor Author

Can you please remind me which use case(s) this addresses?

One of them was mentioned by @ehrlich-uva in #310

In general, it opens possibilities, where this already calculated
information can be used and otherwise would be difficult to
reproduce. And, given that StepPoinMCs are more transient that they
used to be, the cost of having it there diminished.

@ehrlich-uva
Copy link
Contributor

Can you please remind me which use case(s) this addresses?

One of them was mentioned by @ehrlich-uva in #310

In general, it opens possibilities, where this already calculated
information can be used and otherwise would be difficult to
reproduce. And, given that StepPoinMCs are more transient that they
used to be, the cost of having it there diminished.

The module that creates the CrvSteps would use the postMomentum. It currently just estimates it based on the startMomentum and the deposited energy of the step, but without direction information. The startMomentum and postMomentum are used to find the average speed, which is needed for the simulation of the Cerenkov photons and for the calculation of the "postTime" (which would also be nice to have). However, the errors made by not having an accurate postMomentum and postTime are small. The direction of the postMomentum is not needed currently.

@gaponenko
Copy link
Contributor

Can you please remind me which use case(s) this addresses?

One of them was mentioned by @ehrlich-uva in #310

In general, it opens possibilities, where this already calculated
information can be used and otherwise would be difficult to
reproduce. And, given that StepPoinMCs are more transient that they
used to be, the cost of having it there diminished.

I think StepPointMC is a bit of a mess because it is trying to cover too many use cases.
It tries to be both a "point", which ideally should be just a record of particle state at that point (position, momentum,
time, proper time, and a ptr to its SimParticle) and nothing else. Such "points" can be naturally produced
by our infinitesimally thin virtual detectors, and are useful for analysis.
But then there is also the "step", which has things like various kinds of energy deposition, step length, etc.
The "step" information is used by digitization, but different detectors need different infos. For example
visibleEnergyDeposit is of no interest to the tracker. It would be better if each detector produced
their own "steps" without going through StepPointMC, like ExtMon does (see MCDataProducts/inc/ExtMonFNALSimHit.hh )
Then we could have a lean and clean "analysis" object, and a set of detector-defined steps,
without a shared class that has to satisfy everyone.

Andrei

Copy link
Collaborator

@brownd1978 brownd1978 left a comment

Choose a reason for hiding this comment

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

Looks OK to me.

@brownd1978
Copy link
Collaborator

I agree with Andrei's comment. Now that we have DetectorSteps, I think we should rethink the role of StepPointMC. Having a specialized VDStep would be a good start.

Copy link
Contributor

@gaponenko gaponenko left a comment

Choose a reason for hiding this comment

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

I did not like adding postPosition here, and I do not like postMomentum either, but doing things right would take a lot more effort.
I have created #383 to keep track of that.
Code changes in the current PR look straightforward, so I am approving it now.

@kutschke kutschke self-assigned this Mar 20, 2021
@kutschke kutschke merged commit dbd3f0e into Mu2e:master Mar 20, 2021
@resnegfk resnegfk deleted the spmc branch March 22, 2021 23:48
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.

6 participants