Skip to content
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

Introduction of SegMIP BDT + other improvement on ECAL veto #1383

Merged
merged 14 commits into from
Aug 12, 2024

Conversation

tvami
Copy link
Member

@tvami tvami commented Aug 9, 2024

I am updating ldmx-sw, here are the details.

What are the issues that this addresses?

Resolves #1287
Resolves #1305

We have shown the results of the software changes already at a SWAN meeting. The latest BDT that's now trained on the full E14 trigger skimmed data is included now too. This is to be shown at a SWAN meeting.

Future improvements in another PR will include factorizing the MIP tracking out of this class.

Check List

  • I successfully compiled ldmx-sw with my developments
  • I ran my developments and the following shows that they are successful.

@tvami
Copy link
Member Author

tvami commented Aug 9, 2024

We expect the tests to fail, both logs and histogram comparisons.

@tvami
Copy link
Member Author

tvami commented Aug 10, 2024

Error in TFile::TFile: file hist.root does not exist

this should be not related

@tvami
Copy link
Member Author

tvami commented Aug 10, 2024

Nvm, the issue is in the prev line

ERROR: ONNX model file '/home/runner/work/ldmx-sw/ldmx-sw/install/data/Ecal/segmip.onnx' does not exist.

Let me fix this typo

@tvami tvami force-pushed the iss1287-segmip-BDT branch from eb8577a to 16cd33d Compare August 10, 2024 03:17
@tvami tvami force-pushed the iss1287-segmip-BDT branch from 16cd33d to 11816e6 Compare August 10, 2024 03:18
@tvami
Copy link
Member Author

tvami commented Aug 11, 2024

Lots of diffs in the validation, many actually come from a simple change that now the electron beam energy in the ecal veto is configurable, now the plots are correct (meaning consistent) for the signal. For the ecal pn, we need to change the config to be at 8 GeV. I also think we should run tracking in the validation configs. I can do that in another PR.

I propose to do that next and have new golds / subversion change after that so then we can launch a BDT skim.

Copy link
Member

@tomeichlersmith tomeichlersmith left a comment

Choose a reason for hiding this comment

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

As usual, I mainly have some C++ technical comments. I think this implementation looks good and resolves two ( 2️⃣ ❗ ) issues so I'm happy its going to be merged.

I will continue to grumble about #1306 but leave that for future work.

DQM/src/DQM/EcalMipTrackingFeatures.cxx Outdated Show resolved Hide resolved
Ecal/include/Ecal/Event/EcalVetoResult.h Show resolved Hide resolved
Ecal/src/Ecal/EcalVetoProcessor.cxx Show resolved Hide resolved
@tvami tvami requested a review from tomeichlersmith August 12, 2024 19:05
@tvami tvami force-pushed the iss1287-segmip-BDT branch from a83aec8 to 8d1fcfc Compare August 12, 2024 19:08
@tvami tvami requested a review from tomeichlersmith August 12, 2024 20:13
@tvami tvami merged commit 4e76f42 into trunk Aug 12, 2024
2 checks passed
@tvami tvami deleted the iss1287-segmip-BDT branch August 12, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Improved BDT Update EcalVeto BDT for 8 GeV + change hardcoded energy
3 participants