Skip to content

Add LAPPDTreeMaker for EBV2 workflow #340

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.

Sign up for GitHub

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

Jump to bottom
Open
fengyvoid wants to merge 1 commit into
base: Application
Choose a base branch
from
Open

Add LAPPDTreeMaker for EBV2 workflow #340

fengyvoid wants to merge 1 commit into from

Conversation

@fengyvoid
Copy link
Contributor

@fengyvoid fengyvoid commented Jan 20, 2025

EBLoadRaw Tool in EventBuildingV2 tool set.

This is splitted from PR #307 .

Seems you also don't have comments for this one.

@fengyvoid
Copy link
Contributor Author

fengyvoid commented Jan 21, 2025

This build also requires the LAPPDHit.h and LAPPDPulse.h, which were included in previous PR #333

@S81D S81D mentioned this pull request Aug 25, 2025
Closed
@S81D S81D self-assigned this Aug 28, 2025
@S81D
Copy link
Collaborator

S81D commented Sep 8, 2025

As you mentioned we need a re-build to ensure everything works. This should be merged after the other EventBuilding PRs. A quick note, I noticed a few instances of new without delete that should be changed / reviewed (find below)

S81D
S81D reviewed Sep 8, 2025
View reviewed changes
UserTools/LAPPDTreeMaker/LAPPDTreeMaker.cpp
m_variables.Get("MultiLAPPDMapTreeMaker", MultiLAPPDMapTreeMaker);

TString filename = treeMakerOutputFileName;
file = new TFile(filename, "RECREATE");
Copy link
Collaborator

@S81D S81D Sep 8, 2025

Choose a reason for hiding this comment

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

here

S81D
S81D reviewed Sep 8, 2025
View reviewed changes
UserTools/LAPPDTreeMaker/LAPPDTreeMaker.cpp
fGroupedTrigger->Branch("gTrigType", &groupedTriggerType, "gTrigType/I");
fGroupedTrigger->Branch("gTrigNum", &TriggerGroupNumInThisEvent, "gTrigNum/I");

TriggerWordMap = new std::map<uint64_t, std::vector<uint32_t>>;
Copy link
Collaborator

@S81D S81D Sep 8, 2025

Choose a reason for hiding this comment

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

and here

@S81D
Copy link
Collaborator

S81D commented Sep 8, 2025

aside from that, if Marcus did not have any additional comments, I think it's ready to merge.

I don't think it's best coding practice as he's mentioned several times to run toolchain A, then toolchain B, etc... But given the LAPPD complexities with timing offsets, etc... and that this works and has been tested thoroughly, I don't have a good alternative. Perhaps that can be changed in the future.

@marc1uk
Copy link
Collaborator

marc1uk commented Sep 20, 2025

can't see anything new outstanding to mention 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@S81D S81D S81D left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@S81D S81D

Labels

on hold Ready for Level 0 Review and merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fengyvoid @S81D @marc1uk