atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Develop lattice #565

Closed lfarv closed 1 year ago

lfarv commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

swhite2401 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 latticeobject 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 commented 1 year ago

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

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