search

eic / EICrecon

EIC Reconstruction - JANA based
https://eic.github.io/EICrecon
GNU Lesser General Public License v3.0
5 stars 24 forks source link

Setup mc truth UndoAfterBurner #1492

Closed ajentsch closed 1 week ago

ajentsch commented 1 month ago

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.

github-actions[bot] commented 1 month ago

Capybara summary for PR 1492

ajentsch commented 1 month ago

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 https://github.com/eic/EICrecon/pull/1492#discussion_r1628322477:

+#include "extensions/jana/JOmniFactory.h" + +namespace eicrecon { + +class PostBurnMCParticles_factory :

  • public JOmniFactory<PostBurnMCParticles_factory, PostBurnConfig> {

  • +public:

  • using AlgoT = eicrecon::PostBurn; +private:
  • std::unique_ptr m_algo;
  • PodioInput m_mcparts_input {this};
  • PodioInput m_recoparts_input {this};
  • PodioInput m_recoparts_assoc_input {this};
  • PodioOutput m_postburn_output {this};

(note, that works in eic-shell)

— Reply to this email directly, view it on GitHub https://github.com/eic/EICrecon/pull/1492#discussion_r1628322477, 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

ajentsch commented 4 weeks ago

@veprbl is there anything else that needs to be done on this to merge it?

wdconinc commented 2 weeks ago

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.

ajentsch commented 2 weeks ago

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 https://github.com/eic/EICrecon/pull/1492#issuecomment-2187779467, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADABSFZQIIYYJNHXTQS6VNDZJDEVFAVCNFSM6AAAAABIZIMHCCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBXG43TSNBWG4 . You are receiving this because you authored the thread.Message ID: @.***>

ajentsch commented 2 weeks ago

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 https://github.com/eic/EICrecon/pull/1492#discussion_r1652825379:

@@ -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 https://github.com/eic/EICrecon/pull/1492#discussion_r1652825379, 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