Closed xresende closed 1 year ago
@xresende, I don't understand how this PR can change the execution times of the code in the description, because it only adds a new pass_method
, inexistent in the default sirius model. Does it increase the execution times just because of the existence of the new pass_method
?
Another point is that I think this new pass_method
should not be a drift with the global2local
transformations taken into account. I think it could just be a pure transformation of the beam coordinates, with a more appropriate name. What do you think?
@xresende, I don't understand how this PR can change the execution times of the code in the description, because it only adds a new
pass_method
, inexistent in the default sirius model. Does it increase the execution times just because of the existence of the newpass_method
?Another point is that I think this new
pass_method
should not be a drift with theglobal2local
transformations taken into account. I think it could just be a pure transformation of the beam coordinates, with a more appropriate name. What do you think?
@fernandohds564 , as I discussed with you outside github, this PR is a mixture of two possible options: adding g2l back-and-forth transformations in identity and drift passmethods (implemented here!) or adding a new passmethod drift_g2l (also implemented here). I opened this PR so that you guys can answer my posed question: which one is better to keep ?
the time estimates were evaluated using standard drift and identify passmethods with additional g2l and l2g conversions, (as is coded here!), so they do influence the tracking times.... I am confused with you questioning...
regarding you second point: I think we may want to add a coordinate shift or/and rotation in elements that now are drifts in our models. having the possibility of doing this in drifts is the most practical way: avoid changing the model just for adding such transformations in drifts but only changing the passmethod index of the element. also, these drift_g2l pass methods can be used for markers replacement too since de default length=0 is used and generate and identity passmethod in practice. But if computation cost of using a drift(length=0) for markers is to be avoided, we can also add a second new passmethod: one for drift with g2l_transf, another for marker with g2l_transf...
the name drift_g2l
i did not like either, but opened the PR anyways so that you guys can make suggestions. maybe we can change to another thing. g2ltransf
(g2ltransf_drift
and g2ltransf_marker
in case of separate new passmethods) or similar?
I noticed we are not using
t_in
,t_out
,r_in
andr_out
in drift and identity passmethods. This came as a surprise for me when I tried to generate orbit signatures of dipolar perturbations at the center of straight sections (for IDs). Sometimes it is useful to be able to do such a thing, like adding dipolar perturbations at ring locations with such elements (center of straight sections as I did, or at BPMs, for example...), but most of the time applying these global to local transformations and vice-versa are only useless calculations with additional time costs.Another possibility is to add a simple passmethod that can be switched to whenever these transformations are in need for markers or drifts. I also added this possibility in this PR (
pm_drift_g2l_pass
).Which one should we use ?
I measured the additional time cost of running
g2l
andl2g
at all drifts and markers in our current SI model:in this final version
g2l
was removed fromidentity_pass
anddrift_pass
. a newdrift_g2l
was added to allow for phase-space space parameters shift and rotation at elements entrances and exits.