slaclab / lcsim

LCSim Java Library
Other
1 stars 5 forks source link

Detector framework performance improvements #1

Open decibelcooper opened 7 years ago

decibelcooper commented 7 years ago

Three separate improvements are included in this request:

All of these combined tend to speed up digitization by better than an order of magnitude.

EDIT: Also added fix for parent artifact name

JeremyMcCormick commented 7 years ago

What was the extent of testing done to verify these changes? Is there any test case or other program I can run to verify?

decibelcooper commented 7 years ago

Hi Jeremy. Thank you for reviewing this!

The changes in this PR have been verified by examining the digitization and tracking output from DIS events simulated in our SiD-based geometry. Again, there are three independent changes here...

A few materials used in drawing these conclusions can be found here, where the two plots indicate no detectable change in tracking performance over 100 DIS events, and the gzipped tarball contains the text dumps that were diff'd. Certainly, 100 events is not a lot to compare tracking performance, and I believe Sergei Chekanov has made comparisons with high stats, but I don't know if these comparisons are documented.

P.S. I have done a tiny non-functional clean up of the modification to AbstractPolyhedron.java (here) this evening. This has been verified as well to make sure.

EDIT: I also fixed an indentation problem that I introduced here

decibelcooper commented 7 years ago

It is also important to mention that in the test case mentioned above, the digitization+tracking time goes from 18653 ms to 515 ms per event, for a factor of 36 reduction in wall (~cpu) time.

JeremyMcCormick commented 7 years ago

Thank you. This looks nice! I will try to merge this in once I can get around to doing a bit of testing independently on the changes.

JeremyMcCormick commented 7 years ago

Apologies for not responding for awhile. I'm going to need to do some extensive testing with these changes on some downstream projects, so I will import this into an lcsim branch for testing. I may then close this PR until that branch is ready to be merged, as I would like to merge in these changes from a branch rather than fork, assuming everything looks okay.

JeremyMcCormick commented 7 years ago

Changes are copied to the iss13 branch. I will make a new PR from that branch when ready.