-
Notifications
You must be signed in to change notification settings - Fork 29
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
Setup mc truth UndoAfterBurner #1492
Conversation
Capybara summary for PR 1492 |
f622d87
to
01283af
Compare
for more information, see https://pre-commit.ci
I will consider that for the next PR. I think I made all the changes
requested.
…On Wed, Jun 5, 2024 at 3:19 PM Wouter Deconinck ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/factories/postburn/PostBurnMCParticles_factory.h
<#1492 (comment)>:
> +#include "extensions/jana/JOmniFactory.h"
+
+namespace eicrecon {
+
+class PostBurnMCParticles_factory :
+ public JOmniFactory<PostBurnMCParticles_factory, PostBurnConfig> {
+
+public:
+ using AlgoT = eicrecon::PostBurn;
+private:
+ std::unique_ptr<AlgoT> m_algo;
+
+ PodioInput<edm4hep::MCParticle> m_mcparts_input {this};
+ PodioInput<edm4eic::ReconstructedParticle> m_recoparts_input {this};
+ PodioInput<edm4eic::MCRecoParticleAssociation> m_recoparts_assoc_input {this};
+ PodioOutput<edm4hep::MCParticle> m_postburn_output {this};
(note, that works in eic-shell)
—
Reply to this email directly, view it on GitHub
<#1492 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADABSF6ELHOJSG6SPQRBXDLZF5QFZAVCNFSM6AAAAABIZIMHCCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBQGA4TSNRTGE>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
-------------------------------------------------------------
Alex Jentsch
Assistant Staff Scientist, EIC Directorate
Brookhaven National Laboratory
Upton, NY 11973
Bldg. 510, 2-197
Phone (office): 631-344-2139
Phone (cell): 281-726-0114
Pronouns: he/him/his
|
@veprbl is there anything else that needs to be done on this to merge it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are my comments.
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
### Briefly, what does this PR introduce? Adds MCBeamNeutron and MCBeamHadron collections. This matches the current use case in beam.h rather than only providing the protons. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No ### Does this PR change default behavior? No --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
### Briefly, what does this PR introduce? Injects the LowQ2 tracks into the tracking workflow so they can finally be used like any other particle. This might not be the best place to add the information from the events in the longer term as there will may be some extra looping over calorimeter and pid hits which aren't possible. At the moment though there isn't another easy place to add them while there is a mix of ReconstructedChargedParticles and ReconstructedParticles used in different places. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ ] Tests for the changes have been added - [ ] Documentation has been added / updated - [ ] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No ### Does this PR change default behavior? Low-Q2 electrons should show up in ReconstructedParticle collections and easily identifiable in the InclusiveKinematics collections --------- Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com> Co-authored-by: Wouter Deconinck <wdconinc@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this all looks good. It may come as no surprise, but I too prefer the algo name of PostBurn as this describes exactly what we want to accomplish. There may arise cases in which we do not want to explicitly transform to the true head-on frame, but leave in random beam effects and only correct the crossing angle (this is all one can do in data).
I have to agree with @veprbl on the naming here. Postburner literally 1 means afterburner; that's the opposite of what you want here. This name does not make clear what the algorithm does, which is the most important thing we should expect of the name. I understand the hesitation about calling it TransformToHeadOnFrame or so. But what about RevertAfterBurner, UndoAfterBurner, AfterBurnerEffectsRemover, AfterBurnerCorrector, or something like that? Thinking more broadly, even if this does not transform to the head on frame, it does transform or boost to different frames, so names with Transform(er) 2 or Boost(er) 3 should be fine. |
Post means "after", sure, and this is done "after" other operations, so not
the opposite of what we want. This is being rather pedantic. Nothing else
in the ePIC stack uses "burn", and the "afterburner" naming doesn't really
describe what is actually done there, the user has to read a bit to see
what is happening. So, the naming we used was intentional, and ultimately,
the output branch describes exactly what has been done to the information,
as requested in the meetings.
But, it doesn't matter. I just want this merged so we can use it. I will
change it to "UndoAfterBurner" tomorrow and find all the places where I
have to do a search and replace and re-test it again to make sure it works.
…On Mon, Jun 24, 2024, 9:54 PM Wouter Deconinck ***@***.***> wrote:
I have to agree with @veprbl <https://github.com/veprbl> on the naming
here. Postburner *literally* 1
<https://en.wiktionary.org/wiki/post-#Prefix> means afterburner; that's
the opposite of what you want here. This name does not make clear what the
algorithm does, which is the most important thing we should expect of the
name.
I understand the hesitation about calling it TransformToHeadOnFrame or so.
But what about RevertAfterBurner, UndoAfterBurner,
AfterBurnerEffectsRemover, AfterBurnerCorrector, or something like that?
Thinking more broadly, even if this does not transform to the head on
frame, it does transform or boost to different frames, so names with
Transform(er) 2
<https://vignette.wikia.nocookie.net/transformers/images/b/bf/Wfc-bumblebee-1.jpg/revision/latest>
or Boost(er) 3
<https://g-ecx.images-amazon.com/images/G/01/aplus/detail-page/B00BR0OMF6_4.jpg>
should be fine.
—
Reply to this email directly, view it on GitHub
<#1492 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADABSFZQIIYYJNHXTQS6VNDZJDEVFAVCNFSM6AAAAABIZIMHCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXG43TSNBWG4>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Yup - I see that now. I only removed from the global directory. I am
regretting putting this together at all.
…On Tue, Jun 25, 2024 at 9:27 AM Wouter Deconinck ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/algorithms/CMakeLists.txt
<#1492 (comment)>:
> @@ -8,6 +8,7 @@ add_subdirectory(pid_lut)
add_subdirectory(digi)
add_subdirectory(reco)
add_subdirectory(fardetectors)
+add_subdirectory(postburn)
It is still there..This directory should not exist:
https://github.com/eic/EICrecon/tree/setup-mc-truth-postburner/src/algorithms/postburn
.
—
Reply to this email directly, view it on GitHub
<#1492 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADABSFYXLVK5OPQCHKKGLQDZJFV43AVCNFSM6AAAAABIZIMHCCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMZYGY4DCNZUGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
--
-------------------------------------------------------------
Alex Jentsch
Assistant Staff Scientist, EIC Directorate
Brookhaven National Laboratory
Upton, NY 11973
Bldg. 510, 2-197
Phone (office): 631-344-2139
Phone (cell): 281-726-0114
Pronouns: he/him/his
|
4d55406
to
362d395
Compare
df5aa12
to
63c087b
Compare
63c087b
to
bcdefa1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed capybara report https://veprbl.github.io/capybara-reports/00f33c66e1fd0b41a2d5bfdb2468838b/#MCParticlesHeadOnFrameNoBeamFX
Keeping pid_purity and pid_use_MC_truth for future use (use in future RecoParticles version of the algorithm).
### Briefly, what does this PR introduce? This PR introduces the postburner to EICrecon, which will remove the effects of the afterburner from the MCParticles branch, and store the resulting output (the actual MC Truth information from the generator) into a new branch, "MCParticlesHeadOnFrameNoBeamFX". The postburner does not erase any information - the user will simply have access to both the afterburned MC information via MCParticles (as before), and the new branch, which handles the correct transformations to remove the effects. The code is also staged to be able to handle removing *only* the crossing angle from reconstructed branches, and this additional functionality will come with a future PR. ### What kind of change does this PR introduce? - [ ] Bug fix (issue #__) - [x] New feature (issue #__) - [ ] Documentation update - [ ] Other: __ ### Please check if this PR fulfills the following: - [ x] Tests for the changes have been added - [ x] Documentation has been added / updated - [ x] Changes have been communicated to collaborators ### Does this PR introduce breaking changes? What changes might users need to make to their code? No. ### Does this PR change default behavior? [pt_DVCS_protons_MCParticles_POSTBURN_ePIC_eicrecon_6_4_2024_run_0.pdf](https://github.com/user-attachments/files/15570067/pt_DVCS_protons_MCParticles_POSTBURN_ePIC_eicrecon_6_4_2024_run_0.pdf) [Analysis of ePIC Output With Afterburned Events.pdf](https://github.com/user-attachments/files/15570110/Analysis.of.ePIC.Output.With.Afterburned.Events.pdf) It only adds functionality and a new output branch, nothing else is affected. --------- Co-authored-by: Alexander Jentsch <ajentsch@bnl.gov> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com> Co-authored-by: Simon Gardner <simon.gardner@glasgow.ac.uk> Co-authored-by: Wouter Deconinck <wdconinc@gmail.com> Co-authored-by: Sebouh Paul <sebouh.paul@gmail.com>
Briefly, what does this PR introduce?
This PR introduces the postburner to EICrecon, which will remove the effects of the afterburner from the MCParticles branch, and store the resulting output (the actual MC Truth information from the generator) into a new branch, "MCParticlesHeadOnFrameNoBeamFX".
The postburner does not erase any information - the user will simply have access to both the afterburned MC information via MCParticles (as before), and the new branch, which handles the correct transformations to remove the effects.
The code is also staged to be able to handle removing only the crossing angle from reconstructed branches, and this additional functionality will come with a future PR.
What kind of change does this PR introduce?
Please check if this PR fulfills the following:
Does this PR introduce breaking changes? What changes might users need to make to their code?
No.
Does this PR change default behavior?
pt_DVCS_protons_MCParticles_POSTBURN_ePIC_eicrecon_6_4_2024_run_0.pdf
Analysis of ePIC Output With Afterburned Events.pdf
It only adds functionality and a new output branch, nothing else is affected.