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

Update hcal prototype in reduced LDMX #1515

Open
wants to merge 7 commits into
base: trunk
Choose a base branch
from
Open

Update hcal prototype in reduced LDMX #1515

wants to merge 7 commits into from

Conversation

cmantill
Copy link
Contributor

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

What are the issues that this addresses?

This resolves #1512

Check List

@tvami tvami marked this pull request as draft January 17, 2025 21:40
@tvami tvami marked this pull request as ready for review January 17, 2025 21:40
@tvami
Copy link
Member

tvami commented Jan 18, 2025

so when we try to run this inside ldmx-sw we get

Evaluator : unknown variable
Aborted (core dumped)

I looked for hours now which of these could be unknown, but I didnt find it :/

@cmantill
Copy link
Contributor Author

What kind of test is running that gives: "Evaluator : unknown variable"?

@tvami
Copy link
Member

tvami commented Jan 18, 2025

wget https://raw.githubusercontent.com/LDMX-Software/ldmx-sw/e375990bf13c7073634bf2eab3bff7e84a9d991b/.github/validation_samples/reduced_ldmx/config.py
just setenv LDMX_NUM_EVENTS=1
just setenv LDMX_RUN_NUMBER=1
just fire config.py

@tvami
Copy link
Member

tvami commented Jan 23, 2025

@cmantill can you confirm that you can reproduce the error?

@tvami tvami marked this pull request as draft January 24, 2025 18:25
@tvami tvami marked this pull request as ready for review January 24, 2025 18:26
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

OK, so I found two things:

  • the config fails bc of the scoring planes, probably there is some overlap there --> if I switch if off, it works, see 280d315
  • To check the overlap I tried to validated the detector, well, see The validate_detector is broken #1533

I think after the CI finishes I'm ready to sign this

@cmantill
Copy link
Contributor Author

So you are OK with not having SPs for now?

@tvami
Copy link
Member

tvami commented Jan 24, 2025

So you are OK with not having SPs for now?

Yes, unless you see where the problem is already, then let's fix it; otherwise having the SP is less crucial (right @SanjitMasanam , do you use the SP for anything at this point?)

@tvami tvami marked this pull request as draft January 24, 2025 19:41
@tvami tvami marked this pull request as ready for review January 24, 2025 19:41
@SanjitMasanam
Copy link
Contributor

Correct, I don't use the SP for my studies so excluding them will be fine!

@tvami
Copy link
Member

tvami commented Jan 24, 2025

OK it technically works, but we lost a lot of ECAL which makes no sense since only the HCAL was modified, I'm worried it might be because of the overlay
Screenshot 2025-01-24 at 13 38 51

CI results are in https://github.com/LDMX-Software/ldmx-sw/actions/runs/12956457526?pr=1515

@tvami tvami marked this pull request as draft January 25, 2025 03:19
@tvami tvami marked this pull request as ready for review January 25, 2025 03:19
@tvami
Copy link
Member

tvami commented Jan 25, 2025

With the new gold
https://github.com/LDMX-Software/ldmx-sw/actions/runs/12961371139?pr=1515
Red looks like the old blue
Screenshot 2025-01-25 at 09 59 16

I dont really understand why these fluctuate back and forth, but the blue in this PR looks reasonable... We need to look into this in details, but I'm ok to merge this as is for now

@SanjitMasanam SanjitMasanam self-requested a review January 26, 2025 12:12
@tvami tvami marked this pull request as draft January 27, 2025 20:13
@tvami tvami marked this pull request as ready for review January 27, 2025 20:13
@tvami
Copy link
Member

tvami commented Jan 27, 2025

Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

Actually (after Sanjit pointed out), the logs do say a bad config

[ ElectronCounter ] 762  info: Found 1 electrons (tracks) using input collection TriggerPadTracksY_
[ trigger ] 762  info: Got trigger energy cut 1500 for 1 electrons counted in the event.
[ trigger ] 762  info: Got trigger energy sum 5.31643; and decision is pass = true
[ TrigScintDigiPad1 ] 762 debug: Looping over hits in trigScintDigisPad1
[ TrigScintDigiPad2 ] 762 debug: Looping over hits in trigScintDigisPad2
[ TrigScintDigiPad3 ] 762 debug: Looping over hits in trigScintDigisPad3
[ fire ] 763 fatal: [BadConf] : Relative cell coordinates (-85.10, -30.82) mm derived from world coordinates (-30.82, 85.10) mm with layer = 0 and module = 0 are outside module hexagon
  at /home/runner/work/ldmx-sw/ldmx-sw/DetDescr/src/DetDescr/EcalGeometry.cxx:174 in getID

we really need to fix that the CI fails if fire returns fatal, @tomeichlersmith

@tvami
Copy link
Member

tvami commented Jan 29, 2025

OK so we established that w/o this change the ecal part looks fine and run, with this change the ecal geometry is messed up... I do not understand how that's possible when we only add HCAL here...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the HCAL demonstrator into the ldmx-reduced-v2
3 participants