search

atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

Atavedata #678

Closed ZeusMarti closed 1 month ago

ZeusMarti commented 8 months ago

The file has been changed to:

  1. Avoid large errors on small gradient focusing elements (like quad trim coils in sextupoles).
  2. Include orbit generated feed-down effects (orbit at sextupoles).
  3. Include rotation and displacement effects on the focusing strength (displaced sextupoles or rotated quadrupoles).
  4. Include dispersion formula for general combined function magnets (Formula from A.W.Chao Handbook of Accelerator Physics and Engineering Ed.2002, The Wiedemann formula is wrong).

This tries to solve issue #399

swhite2401 commented 8 months ago

Hello @ZeusMarti , this looks nice should we add this to the python too?

ZeusMarti commented 8 months ago

Hi @swhite2401, sure, I can take care of it but I'm not python fluent and we are quite busy with alba-II, it can take some time. Don't take me wrong, I would be happy, for me it would be good a way to force myself to get more familiar with pyat. If you don't feel like waiting for me to go through the exercise please feel free to add the changes yourself. Let me know!

swhite2401 commented 8 months ago

No rush! If you want to train yourself it is very good! Let me know in case you would need any help!

ZeusMarti commented 7 months ago

I tested the two implementations in Matlab and Python. Please take a look at it, it should be ready to be reviewed.

Python avllinopt:

Python_avlinopt

Matlab atavedata: matlab_atavedata_test

lfarv commented 6 months ago

Results look nice. Concerning the code, please look at the review above !

ZeusMarti commented 6 months ago

Results look nice. Concerning the code, please look at the review above !

Sorry Laurent, I do not see the review!

oscarxblanco commented 6 months ago

Dear @lfarv , neither I see your reviews or requests. You might need to finish the review to make it public (something similar happened to me while reviewing a few weeks ago)

lfarv commented 6 months ago

Hi @ZeusMarti and @oscarxblanco. Is it better now? I was confused because for me it appears in the conversation… Sorry.

oscarxblanco commented 6 months ago

Dear @lfarv , yes. Now the review is visible to me.

lfarv commented 6 months ago

@ZeusMarti : I get more that 100 warnings about code not respecting PEP8. Most of them are:

PEP 8: E225 missing whitespace around operator
PEP 8: E303 too many blank lines (3)
PEP 8: E231 missing whitespace after ','
PEP 8: E128 continuation line under-indented for visual indent
PEP 8: E501 line too long (106 > 88 characters)

You should use an IDE which warns about that (ex. PyCharm or VSCode)

ZeusMarti commented 6 months ago

@lfarv , I implemented your last comments.

I realized that I have not properly tested it adding magnet movements, rolls and closed orbit errors, I'll do it after holidays.

Also, the code logic is a bit cumbersome, there are different behaviors for thin or long, focusing or not, drifts. Any suggestions on how to make it more clear? Maybe it just needs more comments?

lfarv commented 6 months ago

Also, the code logic is a bit cumbersome...

Agreed, but since you are at the moment the most familiar with this code, go on… My only concern is about the "epsilon" trick for the focusing strength. Is not it possible to correctly treat the 0-strength quadrupoles?

I realized that I have not properly tested it adding magnet movements, rolls and closed orbit errors, I'll do it after holidays.

Excellent !

ZeusMarti commented 4 months ago

Hi @lfarv,

I'm sorry I have not been able to advance much, however I noticed a peculiar behaviour of the function, both in Matlab and python in the master branch and this branch. Since the input parameter refpts is used only to create a bolean array, if refpts is not sorted that information is lost in the calculation and the results are given always assuming a sorted order.

It was the cause of some trouble for me so I'm wondering if this behavior should be kept. We could simply unsort the arrays at the end or alternatively generate a warning in case of non sorted refpts and output the sorted refpts. Which is your opinion on this?

lfarv commented 3 months ago

Hi @ZeusMarti : the refpts argument, if integer, should always be ordered, and the behaviour you get should be similar in many functions. This is specified in the documentation:

**Selecting elements in a lattice:**

The *refpts* argument allows functions to select locations in the lattice. The
location is defined as the entrance of the selected element. *refpts* may be:

#. an integer in the range *[-len(lattice), len(lattice)-1]*
   selecting an element according to the python indexing rules.
   As a special case, *len(lattice)* is allowed and refers to the end
   of the last element,
#. an ordered list of such integers without duplicates,
#. a numpy array of booleans of maximum length *len(lattice)+1*,
   where selected elements are :py:obj:`True`,
...

The problem is that it would be difficult (and time consuming) to check the validity of refpts everywhere…

Now, for this PR, it's up to you !

ZeusMarti commented 3 months ago

Ah, sorry! My bad. Then I see no point in implementing a different behavior for atavedata.m. hopefully I can address the final details of this PR shortly.

ZeusMarti commented 2 months ago

Sorry it is taking me so long, we have been very busy lately, I tested the scrips again with errors (up to 100um in x-y and 100urad rotation) and the averages hold very well both in Matlab and Python. I think it is ready fore some more review from your side. Matlab: Python_avlinopt_errors Python: matlab_atavedata_test_errors

lfarv commented 1 month ago

Everyone looked happy with this, so I merge