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

ep/eA Performance Test Scripts #486

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

ep/eA Performance Test Scripts #486

wants to merge 3 commits into from

Conversation

Chao1009
Copy link
Contributor

Briefly, what does this PR introduce?

This PR introduces tools to investigate the ep/eA simulation performance issue (currently it is too slow).

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, it only adds tools

Does this PR change default behavior?

No.

@github-actions github-actions bot added the topic: infrastructure Regarding build system, CI, CD label Jul 20, 2023
@Chao1009
Copy link
Contributor Author

Chao1009 commented Jul 20, 2023

An example of the ep test
ff_test

@Chao1009 Chao1009 marked this pull request as ready for review August 25, 2023 18:49
@Chao1009 Chao1009 requested a review from wdconinc August 25, 2023 18:49
@Chao1009 Chao1009 requested a review from ajentsch August 25, 2023 19:09
@ajentsch
Copy link
Contributor

@Chao1009 This is very helpful, but why do the individual detector entries not seem to fill anytime, but "all_ff" does - what is it actually checking?

@ajentsch
Copy link
Contributor

Why does the e_beamline have anything to do with protons?

@Chao1009
Copy link
Contributor Author

Why does the e_beamline have anything to do with protons?

It's the electron beampipe. Sorry for the confusing naming.

@Chao1009
Copy link
Contributor Author

Chao1009 commented Aug 30, 2023

@Chao1009 This is very helpful, but why do the individual detector entries not seem to fill anytime, but "all_ff" does - what is it actually checking?

This is to check the "process time" for each detector component in the farforward.xml with 275 GeV/c protons.
That was for specific theta and phi (theta = 0-5 mrad, phi = 0). The beampipe itself takes too much time so the other components (~ms) appear to be zero height.
I am still figuring out the best angle ranges for the test. Below is the test for theta = 16.0 - 16.2 mrad with phi = pi,

ff_test_275_proton_16_mrad

@ajentsch
Copy link
Contributor

ajentsch commented Aug 30, 2023 via email

@Chao1009
Copy link
Contributor Author

Ah okay, so if you have a significant number of particles hitting the electron beam-pipe, then you likely don't have the crossing angle applied. What input sample are you using for the testing?

I am shooting single particles directly.

@ajentsch
Copy link
Contributor

ajentsch commented Aug 30, 2023 via email

@Chao1009 Chao1009 marked this pull request as draft September 25, 2023 14:17
@kkauder
Copy link
Contributor

kkauder commented Dec 12, 2023

Is there a reason not to just merge this? Doesn't it just add tests?

@wdconinc
Copy link
Contributor

It adds code that isn't tested in CI. Does it work? Does it still work? It isn't possible to tell when it stops working. This fits more in the snippets repository, in detector benchmarks, and only here if integrated in a CI pipeline.

@kkauder
Copy link
Contributor

kkauder commented Dec 13, 2023

That's a good suggestion, I think. @Chao1009 , @ajentsch , can you move these scripts over to https://github.com/eic/snippets?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Regarding build system, CI, CD topic: simulation
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants