Skip to content

MatrixTransferStatic: move matrix configurations to config, implement OMD config for 130 GeV #1780

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

Merged
merged 7 commits into from
Apr 17, 2025

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Apr 2, 2025

Briefly, what does this PR introduce?

This provides an alternative implementation for #1769, without duplicating the code

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No

Does this PR change default behavior?

No

@github-actions github-actions bot added the topic: far-forward Far forward reconstruction label Apr 2, 2025
@veprbl veprbl marked this pull request as ready for review April 2, 2025 04:25
@veprbl veprbl requested a review from ajentsch April 2, 2025 04:25
@veprbl veprbl force-pushed the pr/omd_config branch 2 times, most recently from 4938c7e to dfbe0f0 Compare April 15, 2025 14:44
@ajentsch
Copy link
Contributor

I ran my same set of jobs that I used to verify the other PR to test this one. The resulting plots were empty when I ran my plot maker. Upon further inspection of the EICrecon output, the ForwardOffMRecParticles branch is completely empty. The output from npsim shows the hits, as expected, so the simulation portion is fine.

Screenshot 2025-04-15 at 8 26 30 PM

@ajentsch
Copy link
Contributor

Looking at the logs, the issue is that you removed the ability for the code to process a neutron beam, which was explicitly accounted for in the other PR - it expects all beams here to be protons.

Screenshot 2025-04-15 at 8 31 51 PM

@ajentsch
Copy link
Contributor

Added commit to fix issue. Jobs are running now to test.

@ajentsch
Copy link
Contributor

Some reconstructed output from previous tests with other PR:
Screenshot 2025-04-15 at 8 58 14 PM

Reconstructed output with this PR (now):

Screenshot 2025-04-15 at 10 43 39 PM

There is still a major problem with the final number of reconstructed tracks. At very low-t, the acceptance should be close to 100%, but when I look at a 150k event sample, I only get about 7k total reconstructed protons - I should be getting around 113k.

I have looked at several npsim output files, and the number of detector hits I am getting is consistent with expectations for these events, so it's something else with with this reconstruction approach.

@veprbl
Copy link
Member Author

veprbl commented Apr 16, 2025

In the other PR the values that look like cuts were updated:
https://github.com/eic/EICrecon/pull/1769/files#diff-ca5326a3a427bb0ce8e13f8cdb841f0198c71b2a7f5e26acd778ec80fe71aef1L63
so this one was going to use what's currently in main, no change was made.

@ajentsch
Copy link
Contributor

Yes, the PR I made had updated values for everything, including the matrices for the OMD.

@ajentsch
Copy link
Contributor

Running the jobs again to check.

@ajentsch
Copy link
Contributor

That fixed it:

Screenshot 2025-04-16 at 12 15 30 AM

@veprbl veprbl added the backport v1.24 Backport into v1.24 label Apr 16, 2025
@veprbl
Copy link
Member Author

veprbl commented Apr 16, 2025

@ajentsch I assume, this is okay to backport to 25.04.1?

@ajentsch
Copy link
Contributor

ajentsch commented Apr 16, 2025 via email

@veprbl veprbl added this pull request to the merge queue Apr 17, 2025
Merged via the queue into main with commit f8f8dbf Apr 17, 2025
89 of 90 checks passed
@veprbl veprbl deleted the pr/omd_config branch April 17, 2025 01:58
@epic-capybara
Copy link

Backport failed for v1.24, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin v1.24
git worktree add -d .worktree/backport-1780-to-v1.24 origin/v1.24
cd .worktree/backport-1780-to-v1.24
git switch --create backport-1780-to-v1.24
git cherry-pick -x f8f8dbf9481d112e98169e026f270d1a1674cdc3

@veprbl veprbl restored the pr/omd_config branch April 17, 2025 02:23
@veprbl veprbl deleted the pr/omd_config branch April 17, 2025 02:39
veprbl added a commit that referenced this pull request Apr 17, 2025
…config, implement OMD config for 130 GeV (#1809)

Backport of #1780.

---------

Co-authored-by: Alexander Jentsch <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport v1.24 Backport into v1.24 topic: far-forward Far forward reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants