Lattice operators

[swhite2401]

This PR proposes to introduce function as alternatives to the operators +, +=, *. This allows users to use their favorite syntax (function or operator) but also to document these functionalities in the sphinx webpage.

The default behavior remains unchanged except for the operator * that is now using deep copies of the elements of the original ring for the repetition. All functions are provided with an optional argument copy that allow to use deep copies of the elements to be appended, inserted or concatenated.

This issue was discussed in #565 and reported in #572. There might be other options to achieve this, please advise.

[lfarv]

@swhite2401 :this looks very nice, but I have a naming problem:

The copy keyword argument is already used in many lattice methods like enable_6d, set_cavity, set_value_refpts, add_beamloading and more, with a meaning identical to the inplace keyword of you new methods. On the other hand, your copy keyword has a different meaning (deep copy of elements).

I agree that you convention makes sense, but I think it's too confusing. I suggest element_copy (or anything better) for the new keyword.

[swhite2401]

I agree that you convention makes sense, but I think it's too confusing. I suggest element_copy (or anything better) for the new keyword.

Ok no problem.

By the way, I have an issue with sphinx... I have not sure this can be solved easily. In the lattice_object documentation I have that ALL the method added to lattice_object (find_optics, get_optics, etc....) are document again for lattice_object which makes it very difficult to read (the page is way too long...). Do you think it would be possible to document only the methods present in the module in this part?

[lfarv]

I have an issue with sphinx

I don't think it's possible to filter the methods, apart from writing everything manually. Sphinx imports the Lattice object and documents everything it finds. On the other hand, we also need list of all available methods somewhere…

[swhite2401]

I have an issue with sphinx

I don't think it's possible to filter the methods, apart from writing everything manually. Sphinx imports the Lattice object and documents everything it finds. On the other hand, we also need list of all available methods somewhere…

Ok no surprise then. I was asking just in case, I thought maybe there could be some kind of filter to not should the docstring and instead but a link to the source. Not a big deal

[swhite2401]

@simoneliuzzo you may be interested in this. You can now repeat a cell using cell * n or cell.repeat(n) and elements are automatically copied. Other functions to perform standard operations (append, concatenate,...) are provided that have a copy option too.

[simoneliuzzo]

This looks nice! Do you have also the "-" operator to inverse the order of the elements?

[swhite2401]

This looks nice! Do you have also the "-" operator to inverse the order of the elements?

Ok, after discussion on the meaning of the - operator, this can be added using ring.repeat(n) where n is a negative number. In this case the mirror of the lattice is repeated, taking care of swapping exit and entrance faces of dipoles. Similarly the - operator can be introduced with: ring1 - ring2 = ring1 + ring2[::1] of course properly swapping faces as well.

What do you think? @lfarv ? Any better ideas?

[lfarv]

@simoneliuzzo and @swhite2401 : The "-" operator is not defined for lists, and it would be confusing to define it as "reverse": it is not the reciprocal of addition, ring - ring will not give []. I don't know what could be the reciprocal of concatenation! So for reversing a list, the standard ways are:

• newring = ring[::-1] returns a new object (standard indexing syntax)
• ring.reverse() reverse the order in-place, (standard list method)
• reversed(ring) returns an iterator over the elements is reverse order (built-in python function)

All work for Lattice objects, but don't swap enter and exit faces.

• The ring[::-1] form should be modified, it would break the meaning of indexing.
• The reverse()method can be overloaded and swap the faces. It could also introduce a copy keyword, but the default should be False, to be compatible with the normal behaviour.
• reversed(ring) can be customised by writing a __reversed__() method swapping the faces, but is it useful?

Concerning the use of repeat, I don't like it too much: python explicitly states that ring * n if n is less that 0 returns an empty ring. Can repeat(n) do differently ?

I think we must stick to python standards, so my suggestion is:

• implement the reverse() method reversing in-place with faces swapped,
• optionally implement __reversed__
• set the python behaviour to '*' and repeat: if n is less than 0, take it as zero.

[swhite2401]

@lfarv, I have no strong feeling about this, I think the idea was just to reproduce some of the madx features for building sequences but they are not essential. On the other hand I do not see why we cannot deviate from the List standards. If some features/behaviors are useful for AT they should be adopted regardless of what is done in List. In fact this is already the case with the * operator.

For this PR I will then adpat the .reverse() and reversed() methods to switch dipole faces, this is useful and I do not see why they should behave differently. I will also modify repeat() according to your comments.

[lfarv]

@swhite2401 and @simoneliuzzo : I think there could be a way of modifying the Lattice slice indexing (ring[start:stop:step]) to handle dipole face swapping:

One could decide that in the only case of slice indexing, a negative step value would trigger the face swap. Integer indexing (ring[i]), array indexing (ring[[a, b, c]]) and refpts indexing (ring["QF*"]) are left unchanged.

This still looks reasonable for "getitem", though a question appears for the "setitem": what should a line like ring[0:4:-1] = [drift3, bend2, drift1] do? Swap bend2 faces or not?

[lfarv]

what should a line like ring[0:4:-1] = [drift3, bend2, drift1] do? Swap bend2 faces or not?

Answer: it must swap, so that

ring[start:stop:-1] = ring[start:stop:-1]

leaves ring unchanged !

[swhite2401]

@lfarv why not yes. For me __setitem__ should never swap, the user is responsible to properly set the RHS. Let me first finish with the reverse and reversed devs

[swhite2401]

Answer: it must swap, so that

Then we should give up on this it is too complicated I don't like that AT can change what I am trying to set, it should take my input as is

[swhite2401]

Hello @lfarv @simoneliuzzo , I started looking into this and reversing a lattice is more complicated that it seems if you want to take into account the R1, R2 and T1, T2 attributes and not just face angles and fringe fields. We may decide to ignore them but then the backtracking would not be valid in the presence of alignment errors.... any suggestion? Anything I did not think about?

[lfarv]

@swhite2401 : In any case, the "reverting" operation must be handled by the Element class. The Lattice method should just call elem.revert() or similar.

For T1, T2, R1, R2, I thought about it, and it's probably better to keep them as they are: a magnet shifted "inside" in the direct lattice should better be shifted inside also in the reverted lattice. Same for rotation: I would keep the rotation angle identical. So R1, R2, T1, T2 stay unchanged. This makes sense for "deliberately" displaced magnets. For random errors, anyway the displacements have to be regenerated…

Maybe this intervention on the Element class justified another PR? As you like!

[lfarv]

T1, T2, R1, R2 continuation: one main interest of reversing a lattice is to build symmetrical cells: cell = line + line.reversed(). For this purpose, one must keep T1, T2, R1, R2 unchanged (not swapped) at the element level. If a whole section is displaced, then it's more complicated…

[swhite2401]

Ok thanks.

Well I know people use this to do backtracking and they also use girders misalignments... in this case I think one has to include all geometrical transforms, but this is a very specific case. We can keep this for a future PR. For this one lets keep it simple.

I was indeed adding a method to Element just swapping faces and finge field before I started thinking about this. If we stick to this it is simple enough to be kept in this PR. We will for now ignore misalignment, I will document accordingly.

[lfarv]

In fact, the conversion should be: (T1, T2) -> (-T2, -T1), which means no change in the frequent case of T2 = -T1, but not in the general case. Knowing that any of these may be missing… An Element method can do that

[lfarv]

I was indeed adding a method to Element just swapping faces and finge field before I started thinking about this. If we stick to this it is simple enough to be kept in this PR. We will for now ignore misalignment, I will document accordingly.

Perfect.

Once you have done reverse() and __reversed__() I can take care of indexing in another PR`

[swhite2401]

I can take care of indexing in another PR`

As I said, I am against this proposal, __setitem__ should not modify the users input in any condition (doing a swap or else)

[lfarv]

@swhite2401 : OK, so we keep the slice indexing as it is: ring[::-1] returns the elements in reverse order, without any swapping. No problem for me, it just needs to be consistent !

[lfarv]

T1, T2 again: what I wrote yesterday was wrong. It's more complicated ! When the electron turns back (which is the same as reverting the line), x changes sign (left become right), but y stays the same (top remains top). So only the signs of x and p_x must be changed.

So the procedure is: take T2, change the sign of x and p_x, put it in T1, and take T1 (the initial one), change the sign of x and p_x, put it in T2…

For a simple z rotation (z changes sign), put -R2 in R1 and -R1 in R2.

For the combination of all rotations, it's a nightmare…

[simoneliuzzo]

@lfarv and @swhite2401

I think that the T1, T2, R1 and R2 may be ignored. The '-' feature is mostly used to create a lattice from scratch. Errors will be added after on the lattice already reversed. I doubt that other codes that have the "-" operator (MAD, SAD, elegant, OPA, ...) did deal with errors as well. If we want to protect the user , we may give an error "lattice reverse not allowed with errors" if T1, T2, R1 and R2 are set.

[swhite2401]

Hello @lfarv @simoneliuzzo , I am facong another issue: when an element has been divide in n sub-elements the entrance face is applied to the first sub-element and the exit face to the last sub-element. Reversing the the individual elements does not work in this case.... any ideas on how to handle this?

[swhite2401]

I think I have to treat this on the lattice, for each entrance attribute there has to be an exit attribute (right?) in which case I can collect them all and swap them 2-by-2 regardless of the element they are attached to. If an odd number of faces is found I throw an error

What do you think? Any better idea?

[lfarv]

I do not see why it does not work element by element: taking elements in reverse order, you find the last part of a sliced magnet. Its exit face becomes the entry face of the new "inverted" magnet, its missing entry face becomes a missing exit face. And so on for the previous slices, up to the 1st slice: missing exit face becomes missing entry face, and the original entry face becomes the final exit face. Do I miss something?

[swhite2401]

That is correct, no problem then.

[swhite2401]

Added reverse(), reversed() and some basic tests, I think is ready for merge

[lfarv]

Still waiting for the fix of __reversed__: it must return an iterator

[swhite2401]

Still waiting for the fix of __reversed__: it must return an iterator

@lfarv I replied to this point several times! And I would like to discuss it. I know this is what the list does why should this absolutely return and iterator for the Lattice object? Could you please give your reasons

[swhite2401]

Still waiting for the fix of __reversed__: it must return an iterator

@lfarv I replied to this point several times! And I would like to discuss it. I know this is what the list does why should this absolutely return and iterator for the Lattice object? Could you please give your reasons

I just realized all my comments were not submitted... this is why you did not see them...

[lfarv]

@swhite2401 : You have no choice: the definition of the __reversed__ special method specifies that "It should return a new iterator object that iterates over all the objects in the container in reverse order". The built-in function reversed() is the symmetrical of iter(): both provide a way to get an iterator object (forward or reverse) from an iterable (a list, Lattice or any Sequence). Look at those two special methods in the Python Language Reference

If you want a function returning a Lattice object, you need another one (but it's already there !)

[swhite2401]

@swhite2401 : You have no choice: the definition of the __reversed__ special method specifies that "It should return a new iterator object that iterates over all the objects in the container in reverse order". The built-in function reversed() is the symmetrical of iter(): both provide a way to get an iterator object (forward or reverse) from an iterable (a list, Lattice or any Sequence). Look at those two special methods in the Python Language Reference

If you want a function returning a Lattice object, you need another one (but it's already there !)

It is done, ok to merge?