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

Transformer to create tracks from gen particles #21

Merged
merged 9 commits into from
Aug 8, 2024

Conversation

BrieucF
Copy link
Collaborator

@BrieucF BrieucF commented Apr 26, 2024

BEGINRELEASENOTES

  • Add a transformer to create Tracks out of generated particles
  • Add a consumer that plots the distance between simTrackerHits and a Track
    ENDRELEASENOTES

Copy link
Member

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

Some comments. I didn't run it, but it looks fine

Tracking/components/PlotTrackHitResiduals.cpp Outdated Show resolved Hide resolved
Tracking/components/TracksFromGenParticles.cpp Outdated Show resolved Hide resolved
Tracking/components/PlotTrackHitResiduals.cpp Show resolved Hide resolved
if not os.path.isfile("ddsim_output_edm4hep.root"):
os.system("ddsim --enableGun --gun.distribution uniform --gun.energy '10*GeV' --gun.particle e- --numberOfEvents 100 --outputFile ddsim_output_edm4hep.root --random.enableEventSeed --random.seed 42 --compactFile $K4GEO/FCCee/IDEA/compact/IDEA_o1_v02/IDEA_o1_v02.xml")
Copy link
Member

Choose a reason for hiding this comment

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

Here I'd say it's preferrable to print a message if the file is not there, but always running forcefully ddsim is maybe a bit too much. Note that this will run as soon as the file is processed, before anything related to Gaudi has been loaded or checked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi Juan, thanks for the comments! Not sure I understand this one though. This line is just for convenience (and for tests). So that I can run/test the genTracking just calling one script

Tracking/components/PlotTrackHitResiduals.cpp Outdated Show resolved Hide resolved
@BrieucF BrieucF self-assigned this Jun 25, 2024
Copy link
Member

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

A couple more comments:

@BrieucF
Copy link
Collaborator Author

BrieucF commented Jul 29, 2024

For histogramming I think that works fine, but since a few days ago there is an example in k4FWCore: https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/src/components/ExampleFunctionalTransformerHist.cpp and https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/options/ExampleFunctionalTransformerHist.py where in the cpp file you only need to fill the histogram and the exporting gets done by the RootHistSvc.

Is this really wanted/important? I need to access the TH1 to add the overflow bin

@BrieucF
Copy link
Collaborator Author

BrieucF commented Aug 6, 2024

For histogramming I think that works fine, but since a few days ago there is an example in k4FWCore: https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/src/components/ExampleFunctionalTransformerHist.cpp and https://github.com/key4hep/k4FWCore/blob/971ae2da7efec5a167b4ab6ecf365c0e27ca41ca/test/k4FWCoreTest/options/ExampleFunctionalTransformerHist.py where in the cpp file you only need to fill the histogram and the exporting gets done by the RootHistSvc.

Done

@BrieucF
Copy link
Collaborator Author

BrieucF commented Aug 6, 2024

@jmcarcell the test related to this PR passes (I removed Event Counter). Shall we handle the other tests in another PR?

@jmcarcell
Copy link
Member

In general a different PR is better because then people can look back and understand why the change was done, otherwise they will find this PR...
Another comment is that Associations will have to be updated to Links but this is probably better done at once for the whole repository.

@BrieucF BrieucF merged commit a999d24 into key4hep:master Aug 8, 2024
3 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants