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

Magnetic field offset implementation is off by a sign #278

Open
cameronc137 opened this issue Oct 2, 2019 · 12 comments
Open

Magnetic field offset implementation is off by a sign #278

cameronc137 opened this issue Oct 2, 2019 · 12 comments

Comments

@cameronc137
Copy link
Contributor

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)

  1. Change the magnetic field maps' 4th row, last column number to a non-zero

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

@cameronc137
Copy link
Contributor Author

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:
image

vs. comparing to the results in #277 and #276 with the magnetic field shifted using the new macro command:
image

That uses the same seed, etc. so the only differences come from code changes I suppose

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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).

  • Interpretation 1: The offset value in line 4 of the field map indicates the z position of the origin in the coordinate frame in which the following field points are specified (i.e. offset of 5.087m means that the points with specified z = 9.5m are actually at z = 9.5m - 5.087m)
  • Interpretation 2: The offset value in line 4 of the field map indicates the z shift to apply to the following field points (i.e. offset of -5.087m means that the points with specified z = 9.5m are actually at z = 9.5m + (-5.087m) )

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.

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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.

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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.

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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.

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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.

@cameronc137
Copy link
Contributor Author

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?

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

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.

@rahmans1
Copy link
Contributor

rahmans1 commented Oct 2, 2019

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.

@wdconinc
Copy link
Member

wdconinc commented Oct 2, 2019

@rahmans1 ... let's discuss. Technically not correct but you may be fine.

@wdconinc wdconinc closed this as completed Oct 2, 2019
@wdconinc wdconinc reopened this Oct 2, 2019
@rahmans1
Copy link
Contributor

rahmans1 commented Oct 2, 2019

I did a short vertex check for the geometry that I was running and I didn't see anything obviously wrong. Vertex

@rahmans1
Copy link
Contributor

rahmans1 commented Oct 2, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants