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

1237 add an option to print steps of the daughters of the particle of interest #1273

Conversation

EinarElen
Copy link
Contributor

@EinarElen EinarElen commented Mar 31, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

This resolves #1237 and patches a rare crash with the stepprinter.

  • Add an option to require a minimum energy for the PN products filter
  • Patch a crash in stepprinter when the track is in the world volume and the next step is outside the geometry
  • Add an option to the stepprinter that lets you track any particle produced by a particular process (e.g. "I want to see what happened to all particles from PN interactions")
  • Add an option to the stepprinter to also print descendents of the track of interest up to an arbitrary depth

Check List

  • I successfully compiled ldmx-sw with my developments

  • I ran my developments and the following shows that they are successful.

#!/usr/bin/env python3


from LDMX.Framework import ldmxcfg
p = ldmxcfg.Process('test')

p.maxTriesPerEvent = 10000

from LDMX.Biasing import ecal, util
from LDMX.SimCore import generators as gen
mySim = ecal.photo_nuclear('ldmx-det-v14',gen.single_4gev_e_upstream_tagger())
mySim.beamSpotSmear = [20.,80.,0.]
mySim.description = 'ECal PN Test Simulation'
# Test how the step printer handles printing an arbitrary proces
# step = util.StepPrinter(process_name='photonNuclear')
# Regular tracking of a particular particle
step = util.StepPrinter(track_id=1)
# Track the secondaries of the first particle
step = util.StepPrinter(track_id=1, depth=2)
# step.depth=3
mySim.actions.extend([step])

p.sequence = [ mySim ]

##################################################################
# Below should be the same for all sim scenarios

import os
import sys

if 'LDMX_NUM_EVENTS' in os.environ:
    p.maxEvents = int(os.environ['LDMX_NUM_EVENTS'])
else:
    p.maxEvents = int(sys.argv[1])
if 'LDMX_RUN_NUMBER' in os.environ:
    p.run = int(os.environ['LDMX_RUN_NUMBER'])
else:
    p.run = 1

if len(sys.argv) > 2:
    output_base = sys.argv[2]


# p.histogramFile = f'{output_base}_kind_hist.root'
p.outputFiles = ['output.root']

import LDMX.Ecal.EcalGeometry
import LDMX.Hcal.HcalGeometry
  • I attached any sub-module related changes to this PR.

Related Submodule PRs

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

It seems like the changes to PhotoNuclearProductsFilter are unrelated? I'm not opposed to including them, just wanted to know if this was related to another issue being resolved.

Looks good to me, I haven't tested it yet because I want to check your thoughts on my TrackMap comment before moving forward.

Biasing/src/Biasing/Utility/StepPrinter.cxx Show resolved Hide resolved
Biasing/include/Biasing/Utility/StepPrinter.h Outdated Show resolved Hide resolved
@tomeichlersmith tomeichlersmith linked an issue Apr 1, 2024 that may be closed by this pull request
@EinarElen
Copy link
Contributor Author

It seems like the changes to PhotoNuclearProductsFilter are unrelated? I'm not opposed to including them, just wanted to know if this was related to another issue being resolved.

Looks good to me, I haven't tested it yet because I want to check your thoughts on my TrackMap comment before moving forward.

They were developed together, but I don't think we made an issue for the pn change

@EinarElen
Copy link
Contributor Author

I didn't know that the trackmap stored a complete track/parent history (if it does, I would be a bit concerned about perf 💀)

Copy link
Contributor

@bryngemark bryngemark left a comment

Choose a reason for hiding this comment

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

i approve this pending resolution of tom's suggestion

}

StepPrinter::~StepPrinter() {}

bool StepPrinter::isDescendent(const G4Track* track) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

i vote for the spelling isDescendant, regardless of where this ends up being implemented

Copy link

Choose a reason for hiding this comment

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

Agreed with the spelling!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aren't both of those valid in this context? I'm by no means about to argue about this though, happy to change it

Copy link

Choose a reason for hiding this comment

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

I'm sure there's some semantics on why one over the other - I'm just used to seeing descendant, which is why I agreed with LK's comment. At the end of the day, not sure how much it matters, but maybe LK has a more robust argument that me (:

Copy link
Contributor

Choose a reason for hiding this comment

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

my take was that descendant is a noun and descendent is an adjective. and from the discussion and culturally enforced old habits i suppose i was led to think of descendant on equal footing as ancestor (parent, daughter), and just thought it was a typo. but like you, einar, i won't argue about it 😄 i see your point now.

Copy link

@jpasc27 jpasc27 left a comment

Choose a reason for hiding this comment

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

Looks good! I think these changes will be very useful.

@EinarElen
Copy link
Contributor Author

If we ever wanted an argument for moving away from submodules, I was about to say no to Tom's suggestions just because it would require a separate PR in SimCore :|

@tomeichlersmith tomeichlersmith merged commit 480a963 into trunk Apr 18, 2024
1 check passed
@tomeichlersmith tomeichlersmith deleted the 1237-add-an-option-to-print-steps-of-the-daughters-of-the-particle-of-interest branch April 18, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants