-
Notifications
You must be signed in to change notification settings - Fork 34
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
Field Propagation using Runge-Kutta integration and example15 as demo #166
Conversation
Can one of the admins verify this patch? |
// Also - should we just re-use endPosition / endDirection (in place of midPosition etc ) ?? | ||
|
||
// Check | ||
accurateIntersection = (midPosition - position).Mag2() < deltaIntersectSq; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this check (midPosition - intersectionPos)
? I think at this point position
is still the start point of the chord step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are correct! Well spotted. I will fix it.
I tried to do some testing on my end, here are the results:
compare that to
I tried increasing
|
Thanks for the feedback. |
FWIW the very low energy deposit I reported in #166 (comment) is still there. |
61d6bd8
to
c6d90b3
Compare
I have fixed a key bug in the RK glue code for GPU. I get almost perfect agreement between helix and Runge-Kutta now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave this another try, and after fixing the build failure related to kB2C
in fieldPropagatorConstBz
, it's at least running. However, the energy deposit is still very wrong: With stepLengthFromPhysics = 0.1
, I see around 0.5 GeV for a 10 GeV primary. With that line removed, I get only 3-4 MeV (!).
I also think the CompareResponseVector3D
isn't suitable for comparing directions, which are / should be normalized so their magnitude is always (close to) 1.
examples/Example15/electrons.cu
Outdated
|
||
float BzFieldValue= *gPtrBzFieldValue_dev; // Use vecgeom::Precision ? | ||
if (BzFieldValue != 0.0) { | ||
stepLengthFromPhysics= 0.1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this is from debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, of course. I am making revisions - I had hoped to finish and push them already yesterday, but got sidelined.
I confirm that I have problems on steps which require multiple integration steps at least. I note that the function that compared vectors checked the change from the previous to the next vector - not the magnitude of each vector. So it was not a complete check, but it was a valid check. |
I am still investigating the cause of the errors. They occur when multiple steps are needed to complete an integration. I pushed the code including debugging prints, so that it is in this repository. |
fbdb0ba
to
3b2c27c
Compare
It appears this example is just generally broken, even without magnetic field: Both for |
But wasn't this just copied from one of the previous examples replacing just the field propagator? @jonapost ? |
In example15 I am getting energy deposition which is too perfect when I run with cms2018.gml - a 10 GeV primary deposits exactly 10 GeV, and a 5 GeV primary exactly 5 GeV. Using 10,000 primaries of 10 GeV the results are:
and with 10,000 primaries of 5 GeV:
|
Ooohhhh, I think I see my mistake: All other recent examples default to |
Yes, I agree and will change it back. I had changed it for my testing, but it is simpler and better to have the same default. |
I am working on the rebase. It is a complicated because of the changes introduced in example13, from the use of the RNG to more local variables in place of data structures ... |
@jonapost any progress with the rebase? |
f947388
to
7325e87
Compare
@jonapost looking at the modified code, I see that it is well-localized in |
Actually @jonapost I did the simple exercise and checked out your branch and rebase on the master, which gave no conflict. Then I could even make it compile by changing
|
7325e87
to
f3ccb0b
Compare
- MagneticFieldEquation - DormandPrince45 - Stepper - RkIntegrationDriver - fieldConstants.h with kB2C, delta_intersection parameters - fieldPropagatorRungeKutta : new class for use by electrons.cu, used in new Example14 More: Example15: field propagation using Runge-Kutta. Based on Example13 otherwise. Tests - unit test test_magfieldRK.cpp Simple checks for equation, stepper and driver classes Driver: check version of Advance and V1 (old) Checks driver vs helix results (on cpu). Details: -------- RkIntegrationDriver Introduced new Advance method. Using AdvanceV1 as backward compatible. Enabled used of RK classes with double integrands (field remains float.) PrintFieldVectors: auxiliary methods, initially host-only Changed VECCORE_ATT_HOST_DEVICE to __host__ __device__ example15: Added ability to print Track info check result of RK integration in electrons.cu report differences Optional argument for Bz field value. Use TrackML as default geometry.
… flip side if failing After a maximum number of failed (zero) steps, reducing the step size each time, accept that the track will not cross the boundary, and flip it to the other volume, which it is apparently entering (or never left.)
…field/inc/CompareResponses
boundary; fieldPropagatorConstBz::ComputeStepAndNextVolume - Optional defaul values in interface (at compile time) - Protected some verbosity using if(verbose) - Added some optional verbosity Both of these are to be trimmed / refined.
…puteNextStepAndVolume Enable defaults for the last 3 arguments of ComputeNextStepAndVolume
Safety was const in ComputeStepAndNextVolume implementation only - added it to declaration.
604fdc7
to
1b7f554
Compare
I can run it now successfully with the CMS 2018 geometry for 100,000 initial particles. |
Thanks @jonapost , do you have an estimate timing RK versus uniform field for CMS? |
The CI shows the example15 failing at the moment: https://lcgapp-services.cern.ch/spi-jenkins/job/AdePT-CI/510/console |
Oops, I forgot to commit the change of Stack Limit to 8K. Now done. |
My reading is that it was successful : https://lcgapp-services.cern.ch/spi-jenkins/job/AdePT-CI/511/console |
I am adding the numbers is overleaf: For CMS-2018 and 10,000 electrons (of 10GeV each) the runtime on omenrtx in seconds are:
|
@Japost, I also measured that Exa15 is slower than Exa13 with a factor of 2 for smaller field values and a bit less for 3.8T. I think we can merge this, but we should be careful in quoting the results from this early version before we understand better the main bottlenecks and if we can easily remove some. |
SPDX-License-Identifier: CC-BY-4.0 | ||
--> | ||
|
||
## Example 14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
## Example 14 | |
## Example 15 |
Development of Runge-Kutta integration in 'kernel' code and example15 to demonstrate its use.
New classes that implement the RK integration include:
The development is not yet fully mature.
Runge-Kutta integration has been checked in the unit test vs helix for a uniform field. The classes to integrate it into this example are created, and initial tests done.