search

eic / EICrecon

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

Reconstructed inclusive kinematics algorithms now use centralized algorithms for scattered electron ID and hadronic final state calculation #1453

Closed tylerkutz closed 1 month ago

tylerkutz commented 2 months ago

Briefly, what does this PR introduce?

All reconstructed inclusive kinematics algorithms now obtain:

Previously, these steps were repeated as-needed in each individual inclusive kinematics algorithm. Now, improvements/fixes made to ScatteredElectronTruth or HadronicFinalState will automatically be included in the inclusive kinematics algorithms.

This also fixes issue #1318 , as the HadronicFinalState algorithm uses all reconstructed particles, not just reconstructed charged particles.

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, although the specific values of the reconstructed inclusive kinematics might be different (see below) the output collections are in the same format.

Does this PR change default behavior?

The electron reconstruction method should be identical, as the only difference is where the scattered electron is found.

The reconstruction methods using any information from the hadronic final state (JB, DA, [e]Sigma) will be different, as the hadronic final state now includes all reconstructed particles, not just charged particles.

simonge commented 2 months ago

Any chance you also fancy swapping out MCParticles and using MCBeamElectrons and MCBeamProtons instead? This will stop the algorithms from independently needing to carry out the search, it could also be restricted to call once as it doesn't change between events, although I'm sure none of this is very expensive.

Ideally the information needed from the MCParticles would be contained in the metadata and used in the initialisation.

tylerkutz commented 2 months ago

@simonge Great suggestion, I was actually not aware of these. I will switch to using these instead.

veprbl commented 2 months ago

I agree with @simonge's suggestion. If this is not implemented yet, can we, please, hold this off for another PR? This would ease the review process a bit.

veprbl commented 2 months ago

My proposal is to wrap the 4 .cc files in src/algorithms/reco entirely in

#include <edm4eic/EDM4eicVersion.h>
#if EDM4EIC_VERSION_MAJOR >= 6
// SPDX-License-Identifier: LGPL-3.0-or-later
// Copyright (C) 202....

// contents of the file
#endif

and do some more careful guarding of includes and factories in reco.cc. The header files would end up unincluded as the result of this. This will disable kinematics factories for EDM4eic 5, but would allow to still test everything else. It would be easy to revert that later, once that's deprecated. @tylerkutz, what do you think?

github-actions[bot] commented 2 months ago

Capybara summary for PR 1453

veprbl commented 1 month ago

Saving capybara report for the current state (c6fa0447df64af2f1fb25f5f67c7df3f6f4a76aa) to https://veprbl.github.io/capybara-reports/a664ff9eb9cd58755d862771ff2bab0f/

veprbl commented 1 month ago

As discussed in the meeting today, let's merge this and limit changes to refactoring and switching from ReconstructedChargedParticles to ReconstructedParticles. Other changes/reverts can follow.