-
Notifications
You must be signed in to change notification settings - Fork 55
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
Magnetic field offset implementation is off by a sign #278
Comments
Wouter has fixed this in #277 and #276. Confirmed by comparing 10000 events generated with Moller generator in an old version of develop (4d2d029) which yields the following: vs. comparing to the results in #277 and #276 with the magnetic field shifted using the new macro command: That uses the same seed, etc. so the only differences come from code changes I suppose |
I would not call this a bug, and rather a difference in interpretation. This is also not related to PRs #276 and #277 (which keep the field maps unchanged, with a 0 in line 4 of the field map).
The code assumes interpretation 1. I think we should keep the behavior (and field maps) unchanged to the extent possible and improve the clarity of the documentation. |
There is a trade-off between modifying the field when reading it in (i.e. once, not every event, but at the cost of reduced flexibility when running) and when getting a field value (i.e. every step, but allowing easy studies of shifts, rotations, etc). For a z offset shift this is just an addition (subtraction), so doing this for every step is peanuts compared to the multiplications in the interpolation. For a phi rotation it is less clear, since cos/sin are slow, and there are multiple multiplications to apply the rotation in x and y. |
Moreover, none of the positions in the set of points in the field map are actually used (yeah, the field maps are twice as large as they need to be due to the inclusion of r,phi,z coordinates). What determines the grid point is the order in the file, and the n/min/max grid specification lines at the top. In that sense, you don't even need the offset lines since you can just change the respective min/max line. |
So, for whoever designs field map routines for the next experiment, these are considerations to keep in mind. Also: storing numbers as strings is inefficient (factor 20 space using default gzip, probably 30 using other compression), reading numbers as strings is slow (creating a binary field map file will increase read-in speed 20 fold as well). After optical photon tracking, the propagation of charged tracks in magnetic fields is where simulations spend most of their time, in particular in the interpolation routines. |
So, in summary, I'm going to close this bug in a few days after those interested have had a chance to read my comments. This is not the bug that needed fixing. The sign bug in #276 was in the beam/target class where the target position is turned into a primary vertex position. |
Is the best solution to rely on this new translation feature for the map file's origin? If so it may be good to also make sure that the default value of that offset is this 5.087 value, so people don't get burned by it if they forget/aren't aware of the macro command? |
If we change the field map, then everyone needs to download new files, we need to update md5s etc, which we should try to avoid. So, we can set a default value. Good idea. |
If I understand this correctly, if the zero of the mother coincides with the zero of the logical target as it did before, the sims aren't affected by either of the problems mentioned above. |
@rahmans1 ... let's discuss. Technically not correct but you may be fine. |
My understanding is that the field maps directly correspond to the global coordinate system with the logical target at 0. So, as long as 6000 mm in the map corresponds to 6000 mm (where the upstream torus roughly begins) in the simulation defined as such it should be fine. But with translated geometries we have to be more careful. |
Environment: (where does this bug occur, have you tried other environments)
The magnetic field offset implementation has always been off by a sign.
Steps to reproduce: (give a step by step account of how to trigger the bug)
Expected Result: (what do you expect when you execute the steps above)
It should shift the field maps in z
Actual Result: (what do you get when you execute the steps above)
It goes the wrong direction
The text was updated successfully, but these errors were encountered: