Develop lattice

[lfarv]

This PR introduces a new method of the Lattice class, which converts a periodic ring into a ring with a single period repeating the superperiod n times.

Plus small changes to make the import order simpler

[swhite2401]

I understand you enforce that periods divides periodicity in order to allow to set a new periodicity, this in my opinion is over constrained.

I would suggest to develop in any case and ONLY if rem==0 set periodicity=newper otherwise set it to 1.

Also this function will never work if periodicity=1 which is used very often, this sould be allowed and return a new Lattice with periodicity=1

[lfarv]

Also this function will never work if periodicity=1

It works, but does nothing, except making a deep copy of all elements.

Apparently, there is a misunderstanding about what "develop" means: by definition, it applies to periodic lattices (n>1). In the general case, to reproduce a lattice n times, one should use the '*' operator: newring = ring * n. The behaviour is there identical to what you propose: change the periodicity attribute if possible, otherwise set it to 1 with a warning.

Otherwise, if you suddenly decide that a lattice is periodic, you may simply set its periodicity to the number you want. And than develop it or not.

To be more clear, I'll suppress the "periods" optional argument. There will be no ambiguity and no need to test for the remainder. The only drawback is to prevent a "partial" development, which is rather unlikely to be needed.

[swhite2401]

This optimization should be ok but is really a minor improvement, have you tested it?

Generally speaking: this is completely unrelated to this PR. As you mentioned in the past it is not clean to mix bugfixes/optimizations with developments otherwise it gets very confusing for the reviewers

[lfarv]

Generally speaking: this is completely unrelated to this PR

No, it's meant to keep the filling pattern unchanged when developing the lattice

[swhite2401]

Using a fill pattern on a partial lattice makes absolutely no sense, why are we providing function that make it possible?

[lfarv]

Principally it should work, the periodic and developed descriptions of the same lattice should behave identically ! At least the bunch assignment works. For the hmba test lattice (periodicity=32), I get:

>> hmba.circumference
843.9772144741422
>> hmba.harmonic_number
992
>> hmba.set_fillpattern(16)
>> hmba.bunch_list
array([  0,  62, 124, 186, 248, 310, 372, 434, 496, 558, 620, 682, 744,
       806, 868, 930])
>> hmba.bunch_spos
array([  0.        ,  52.7485759 , 105.49715181, 158.24572771,
       210.99430362, 263.74287952, 316.49145543, 369.24003133,
       421.98860724, 474.73718314, 527.48575905, 580.23433495,
       632.98291086, 685.73148676, 738.48006266, 791.22863857])

All this is correct.
Then in tracking, there should be no difference between the 2 descriptions.
So I do not know where the problem is. If there is indeed something wrong, and if the fix is too complex (I agree that it's not worth spending time on that, the develop function may be used), you may consider preventing setting the fill pattern on periodic lattices. But that is for another PR !

[swhite2401]

you may consider preventing setting the fill pattern on periodic lattices. But that is for another PR !

In fact I did consider this when I introduced fillpattern .... but did not in the end thinking that no one would ever think of using this for periodic lattices

So I have 2 questions:
1- is fastring properly accounting for periodicity, if not it should be included in the new refurbished version
2-you say in tracking there is no difference? Do you infer that tracking one cell with periodicity=32 or 32 cells will give the same results? Because this I do not understand at all....

The reason behind these questions: the only usage of fillpattern is for tracking and mostly with fastring.

[lfarv]

Point 2 first: tracking takes elements one after the other, independently of how they are grouped in a lattice object. So tracking a periodic lattice or a developed one is strictly identical, but of course the notion of "turn" is different. 32 "turns" of a periodic lattice is the same as 1 "turn" of the developed one. This is a detail, which could even be hidden within lattice_pass, though I would see it confusing. The important point is that the absolute path length is correctly computed in all cases.

Point 1 now: interesting point ! Grouping a number of elements in a single transfer matrix is arbitrary. The longer the group, the faster the tracking, but on the other hand, you loose in accuracy. Computing a fastring from the periodic lattice or from the developed one are different, but both valid. Note this: an electron travelling in a periodic lattice sees identical cells repeating again and again, but it has no notion of "turn". The 32nd cell (in my example) has nothing special compared to the 31st or the 33rd. The turn has no existence for the electrons in a periodic lattice. So fastring could as well group 31 cells or 33 or 64 (2 turns) or more, it's completely arbitrary. Forgetting the transverse motion, which is severely affected by the grouping, I would think that the grouping should be small compared to the synchrotron period and to the growth time of any potential instability. Concerning impedances, of course the impedance element must sum up the impedances of the whole "group", whatever it is.

So I don't see anything wrong for the moment. One could improve with another argument of fastring giving the number of cells to be grouped, with a default of Lattice.periodicity so that the default is one "geometrical" turn, but would allow grouping several turns. This could be combined with a refurbishment of fastring previously mentioned by @lcarver, but this is outside the scope of this PR. I have something somewhere in my files, but it's on hold since long.

If there is however a problem somewhere, it's probably not specific to collective effects, so if you have doubts, let us know…

[swhite2401]

Continuation of the 2 points:

1-Ok for the basic tracking, I have no problem with that, I am concerned with the fillpattern and more generally collective effects and its usage in tracking. The turn number and ring.circumference are used to shift the slices at each turn. I am afraid that this logic breaks with periodic lattices where ring.circumference is the one of the developed turn while the whole tracking assumes a turn that is the length of the period. I think this cannot work and ring.circumference should be replace with period length instead but it is not accessible in atpass right?.

2-Again for the tracking it should be ok but fastring generation uses quantities such as chromaticity and amplitude detuning. I am afraid again that these will end up being the ones for the full ring (this the whole point of periodic lattices!) which is wrong in this case. I would be very much in favor of always computing the fastring for the developed lattice to prevent any issue.

Other than that not issue really, I remember we had this conversation on periodicity long ago and you know my opinion on it no need to come back to this.

[lfarv]

I think this cannot work and ring.circumference should be replace with period length instead but it is not accessible in atpass right?.

You are right, but I think atpass is correct: the variable names are misleading but the C "param" structure contains:

• RingLength, which is in fact the cell length, or period length, recomputed in C by atpass from the input cell,
• nturn, which should have been called npasses, the number of passes through the cell, also internally handled y atpass,
• 'T0', the "cell revolution" time.
• …
  All these are consistent, otherwise RFCavityPass wold not work. It's OK for longitudinal motion, and should be OK for shifting the slices, either on each cell for periodic machines, or one each turn for developed ones.

[lfarv]

fastring: it is also consistent: it uses the cell chromaticity for periodic machines, and the ring chromaticity for developed ones. However, I have to think about it for amplitude detuning…

I would be very much in favor of always computing the fastring for the developed lattice to prevent any issue.

My proposal for an additional argument (see above) would implement that !

[swhite2401]

Ok then all set it seems. I have put a comment to remember this in the fastring issue

[swhite2401]

@lfarv, this looks ok to me beside the 2 comments above.
Could you please implement unit tests for this feature? We had agreed with @simoneliuzzo to be a bit more serious on these

[lfarv]

Added test for Lattice.develop: check that circumference, rf frequency, rf voltage, tunes, energy loss are equal for periodic and developed lattices.

[swhite2401]

@lfarv, why not use this as an occasion to provide a user friendly interface to ring * n ? I don't think a lot of people will think of using this by themselves, in fact I would have not. Not everyone is an expert python developper....

I think a well documented function would be useful. This one is well suited and I liked the period argument. Why limit the scope of this function? Users in general prefer a function in my experience...

[lfarv]

@swhite2401:

Users in general prefer a function in my experience...

I just have the opposite experience: using language features is always better and easier to understand. And operators are cleaner than function calls. Look at the old python 2 syntax for matrix multiplication: numpy.dot(a, numpy.dot(b, c)) compared with the python 3 syntax: a @ b @ c ! The * operator for lists is documented and well-known. Would you also need a function for the + operator (Lattice concatenation) ?

Anyway, if you think it's useful, we can create 2 new Lattice methods for that: add() and multiply() (or reproduce()) for instance. But there is a difference with the develop method: develop returns a new Lattice which is equivalent in all respects to the original one: see the new test_develop function in test_lattice_object.py. On the other hand, + and * create new, different lattices. This is why I prefer having separate functions, and no argument for develop.

Can we keep this for another PR ?

Note: there is anyway something bad in the * operator: unlike develop, it does not make copies of the elements. We should change that, there is no interest in having all the lattice periods linked together. Again for another PR.

[swhite2401]

I just have the opposite experience: using language features is always better and easier to understand.

This is not what I have seen first hand with many users, I have seen them using numpy.repeat() loop, etc.... but very rarely the operators in fact. Maybe because their usage on the lattice object is not well documented. Remember that many users of AT are not python experts and do not necessarily associate the lattice object with a list or do not know what operation can be performed on a list.

The main advantage of having function is that you can document them, including the features you added w.r.t. standard lists such as changing the periodicity and the help may encourage users to use operators (this is what is done for numpy.dot() I think).
So I agree with you proposal, this doesn't necessarily have to be in develop and let's keep it for another PR, including the issue you mention with copies. I'll put an issue in to make sure we remember about it

[lfarv]

I'll put an issue in to make sure we remember about it

Perfect, this way everybody may pick its preferred coding style !

[swhite2401]

Ok for this part, problems arising in the discussion will be treated in other PRs