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

Neutron energy scale #1454

Closed sebouh137 closed 2 months ago

sebouh137 commented 2 months ago

Briefly, what does this PR introduce?

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?

yes. Changes default behavior of the FarForwardNeutronReconstruction algorithm to include energy corrections.

github-actions[bot] commented 2 months ago

Capybara summary for PR 1454

sebouh137 commented 2 months ago

@veprbl , IWYU is trying to get me to change one of the include statements to use a header that does not exist.
In src/global/reco/reco.cc, it wants me to change #include <JANA/JApplication.h> to #include <JANA/JApplicationFwd.h>, however, doing so causes the compiler to complain that this header does not exist. What's going wrong here, and can we override this somehow?
Best regards, Sebouh

veprbl commented 2 months ago

@sebouh137 Good observation. Let's not use the Fwd header just yet. It breaks compilation in older containers, it seems like.

sebouh137 commented 2 months ago

@veprbl , the only checks that are failing are the get_docs_from_main and eicweb/eic_container. Are there things that I need to fix on my end to make this branch pass these checks? If not, let me know anything else I need to do in order to get this pull request approved. Thanks, Sebouh

veprbl commented 2 months ago

It would be nice to have that fixed. I'm trying to fix it via #1459

veprbl commented 2 months ago

HcalFarForwardZDCNeutronCandidates doesn't show up in capybara. Is it produced at all?

ruse-traveler commented 2 months ago

HcalFarForwardZDCNeutronCandidates doesn't show up in capybara. Is it produced at all?

It should be in ReconstructedFarForwardZDCNeutrons... Which no longer seems to be listed in JEventProcessorPODIO.cc?

veprbl commented 2 months ago

https://github.com/eic/EICrecon/pull/1404/files#diff-d750a62ff8386199706241b174c0b6466ec5714a43057a4fd591df3dd77de239R290 vs https://github.com/eic/EICrecon/pull/1404/files#diff-08c369ce77f61e0738a1b6bea175ebb10a4df985a97ced435296883bcca0992cR279

sebouh137 commented 2 months ago

HcalFarForwardZDCNeutronCandidates doesn't show up in capybara. Is it produced at all?

It should be in ReconstructedFarForwardZDCNeutrons... Which no longer seems to be listed in JEventProcessorPODIO.cc?

I must have forgotten to change the name of this bank in JEventProcessorPODIO.cc during the review of the earlier pull request. Just fixed this now.

ruse-traveler commented 2 months ago

I must have forgotten to change the name of this bank in JEventProcessorPODIO.cc during the review of the earlier pull request. Just fixed this now.

Thanks! Let's wait for the capybara, and then if that looks fine, I think this is good to go... Are there any concerns you have @veprbl?