atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

lattice_pass modifies initial coordinates #564

Closed lmalina closed 1 year ago

lmalina commented 1 year ago

The lattice_pass method is directly modifying the initial coordinates r_in without ever returning them. For example, the following code modifies Zin inside latice_pass: Zin = SCgenBunches(SC) T = latice_pass(SC.RING, Zin, nTurns, refOrds, keep_lattice=False)

This is clearly unexpected behaviour. From the fact that it is documented in the docstring, however, I assume it is intended to get the final coordinates "out of" latice_pass. If this is the case, I would suggest properly returning the final coordinates (possibly steered by a flag) without modifying the initial coordinates.

swhite2401 commented 1 year ago

Hello @lmalina ,

r_in is a python object and is modified in place just as it is done in the underlying C code. This is not unexpected, however documentation could be improved.

In the short term to fix it you may use: T = lattice_pass(SC.RING, Zin.copy(), nTurns, refOrds, keep_lattice=False)

On the longer term, we may add the flag as you suggest to use a copy of r_in or not but changing the output format is strongly discouraged as it may impact other users codes and internal AT functions.

However, there is a need to refurbish the tracking function in my opinion right now it is a bit spread out between lattice_pass, patpass with variable outputs whether you activate losses or not, etc... A good clean up would help and your request could be integrated.

@lfarv, what do you think? I would like to suggest introducing a new general tracking function (keep the old ones for backward compatibility) and integrate to the output a structured array as is done in get_optics that would allow the function to evolve and integrate request as this one without changing its interface.

lfarv commented 1 year ago

@lmalina and @swhite2401 : lattice_pass is such for efficiency reason, and as @swhite2401 mentioned, we should not change its interface because it's the basic tracking function used in many places. The documentation clearly states that r_in is modified in-place. This allows for instance to perform a long tracking by repeatedly calling lattice_pass for one or a few turns: this way, you can monitor the tracking turn-by-turn, without needing a huge space for the output of the many turns. By putting refpts=[], you even suppress all output ! And this avoids any data copy.

But I agree that a new function on top of lattice_pass could offer a more convenient interface, wile keeping lattice_pass where performance is critical.