Cleanup Lattice and cavity access

[lfarv]

The Lattice object has new methods an properties:

get_slip_factor
get_revolution_frequency

and corresponding read-only properties:

slip_factor
revolution_frequency

The cavity access module is improved, particularly on frequency: it is possible to set the frequency to the "nominal" value, optionally specifying an off-momentum.

All this is now explained in a new page of the Web site. To allow previewing this page, the site is temporarily directed to this development branch. Please look at it, in particular to the frequency control, and give your comments, either on the code or on the documentation.

NB: I had to change the name of frequency controls, to clearly distinguish revolution_frequency and rf_frequency. If this is not acceptable, I can revert to initial (frequency in short).

[lfarv]

Other additions:

• the constants.py module grouping all physical constants is implemented,
• Matlab documentation for cavity access. Please look at it, on the Web site and also in the Matlab help browser

[swhite2401]

This looks good, I have question though: why are you storing the particle mass in constats.py while other quantities are initiated in the __init__.py of physics?

Also variable names like clight are used by many, are not afraid that this gets over-written by mistake? Don't you want to call these variables atclight etc... instead?

[simoneliuzzo]

in the help of atsetcavity I read:

%   The harmonic number of each cavity is HARM_NUMBER / N_CELLS

is this intended? may be just a copy-paste.

[lfarv]

This looks good, I have question though: why are you storing the particle mass in constats.py while other quantities are initiated in the __init__.py of physics?

Laziness ! Still a few modules to update… I'll move them. Already 26 files changed, it makes reviewing tedious, but maybe a few more does not matter.

Also variable names like clight are used by many, are not afraid that this gets over-written by mistake? Don't you want to call these variables atclight etc... instead?

I prefer naming constants with short and commonly-used names. Using clight for something different would be crazy, and if it is for the same thing, it makes the conversion easy. And I would not like to suggest that AT has its own view of the speed of light. But I admit that I found the single 'c' provided by Scipy really too dangerous, for the reason you mention !

[swhite2401]

Well I often write clight=2.998e8 in my scripts because my mind doesn't seem to be able to remember more than 3 digits, in which case it will affect the results...

[lfarv]

in the help of atsetcavity I read:

%   The harmonic number of each cavity is HARM_NUMBER / N_CELLS

is this intended? may be just a copy-paste.

I think this is right: if you describe one cell of a ring, it has to be self-consistent, so that you can track it independently. What AT sees, tracks and analyses is the "cell". The "ring" values are for convenience, because they are the ones commonly used (think of tunes, circumference, and indeed harmonic number). So you have:

circumference_cell = circumference_ring/n_cells
rev_freq_cell = rev_freq_ring*ncells
rf_freq_cell = rf_freq_ring
voltage_cell = voltage_ring/n_cells
harm_number_cell = harm_number_ring/n_cells
tune_cell = tune_ring/n_cells
mcf_cell = mcf_ring
and so on…

All relationships between these variables must apply on both cell and ring.

To answer one of @swhite2401's question on the usefulness of the harmonic number, it's for instance useful to compute the nominal RF frequency. I admit that it could be an attribute of the lattice rather than RF cavity,

[swhite2401]

Honestly, I find this periodicity (or ncells) property of the lattice extremely confusing and it leads to errors for non-expert users. My personal opinion is that it should be an argument of ringsummary/ringparam such as to get characteristic quantities like emittance or energy loss per turn based on a single cell and nothing more. Until now, from my users point of view, it has only been a source of errors.

Reading the new help of atsetcavity I would set the Harmonic number to HarmNumber/ncells even for a the full ring with periodicity=1, this is not good.... I would very much prefer if all these set functions apply the value given as argument without any "silent" modifications, the user would then be responsible to properly scale them for periodic lattices.

I understand periodicity is a historical parameter than could have been useful to speed up some computations in the past but is it really still the case?

[lfarv]

Reading the new help of atsetcavity I would set the Harmonic number to HarmNumber/ncells even for a the full ring with periodicity=1, this is not good....

if periodicity (or ncells) is 1, it sets the Harmonic number to HarmNumber. Is not it what you want ?

I think you are misunderstanding the meaning of periodicity. periodicity is the number of times the lattice description is repeated to get a full ring. So here are a few hints:

• if you are dealing with non-periodic machines, you describe the full ring, set periodicity to 1, and AT does exactly what you ask for: all input are set unchanged to the lattice elements,
• it often happens that in the early design stage, we consider a ring as periodic. Then you describe only one cell, and set periodicity to 32 (for instance). Then hundreds of details break the periodicity, you describe the full ring, and set periodicity to 1. Thanks to periodicity, both machines have the same circumference, tunes, revolution frequency, harmonic number etc.
• If in spite of this, you still don't like it, always set periodicity to 1! If you describe 1 cell, you get 1 cell, if you describe the full ring, you get the ring. It's up to you to convert the parameters between both.

It's not a question of speed. It just makes the description of periodic machines easier by describing only one cell and say "repeat it n times".

Of course it may happen that periodicity is not properly taken into account in the code. The solution is to repair it, not to remove it.

[simoneliuzzo]

Dear @lfarv and @swhite2401 ,

I also find periodicity rather misleading. It is question 0 of everyone starting to use pyAT. What is periodicity? how to set it? answer is often: please set to 1 and forget about it.

I personally fought with this feature also with AT1.* . I was allowed to work with a single cell, but I add to set unreal parameters for the harmonic number to get the correct parameters. Quite confusing, personally.

I think that it is more user friendly, to let the user multiply is lattice by the relevant amount of cells to get a full ring.

To allow to have computed parameters for a full ring starting from a single standard cell, I think that the suggestion of @swhite2401 to input periodicity in atsummary, or ringpara and similar functions, is more convincing and of intuitive use (if I understand it well).

Those functions could have periodicity =1 and if need be, "autocompute" the number of cells from the angle, or use the user input.

[lfarv]

@swhite2401 and @simoneliuzzo : I'm really puzzled by your confusion !

Is there any ambiguity on the number of times you have to repeat your cell to get a full ring ? I don't see why! That's periodicity. Is there a better way to explain this in the documentation ?

Is there any ambiguity on the harmonic number ? It's the ratio between the RF frequency and the revolution frequency of the ring. That's what you have to set as harmonic_number. Again, of you find how it can be better expressed, tell me.

So where is the difficulty ?

Anyway, if you don't like it, you both propose a way of working which is perfect legal : set periodicity to 1 and do all the scalings yourself. Why not, some of the rules are given above.

Another way which is to describe a ring as the concatenation of n identical cells is also possible, by I find it too bad to multiply all the computation times (orbit, matrices, optics) by n, and loose on numerical errors. A factor 32, to take a well known example. That's not marginal ! But you can do it !

But you should not prevent users who want it to use it : you can easily choose the way you want to work.

[lnadolski]

I do agree with @lfarv
Periodicity is an import feature to keep.
If there is confusion, maybe the documentation should be improved.

[lfarv]

2 questions about constants.py:

1. Well I often write clight=2.998e8 in my scripts because my mind doesn't seem to be able to remember more than 3 digits, in which case it will affect the results...

   So there is an easy way to update your script: remove the line clight=2.998e8 and add from at import clight. All the rest of the code does not change and you get a more accurate result. Nice, isn't it ? True, there is a problem if you have both lines, but why ?

2. At the moment, all the constants are in the lattice namespace, and consequently in the at namespace, so that you can get clight (for instance) as one of:

   from at import clight
   from at.lattice import clight
   

   We can isolate them a bit more by adding only the constants module to the lattice namespace. Then, to access clight, you would need to use:

   from at.constants import clight
   from at.lattice.constants import clight
   

   Do you have a preference ?

[swhite2401]

2 questions about constants.py:

1. > Well I often write `clight=2.998e8` in my scripts because my mind doesn't seem to be able to remember more than 3 digits, in which case it will affect the results...
   
   
   So there is an easy way to update your script: remove the line `clight=2.998e8` and add `from at import clight`. All the rest of the code does not change and you get a more accurate result. Nice, isn't it ? True, there is a problem if you have both lines, but why ?

2. At the moment, all the constants are in the `lattice` namespace, and consequently in the `at` namespace, so that you can get `clight` (for instance) as one of:
   ```
   from at import clight
   from at.lattice import clight
   ```
   
   
       
         
       
   
         
       
   
       
     
   We can isolate them a bit more by adding only the constants module to the  lattice namespace. Then, to access `clight`, you would need to use:
   ```
   from at.constants import clight
   from at.lattice.constants import clight
   ```
   
   
       
         
       
   
         
       
   
       
     
   Do you have a preference ?

What you have at the moment is fine, still I think your implementation is not safe and cannot guaranty that clight (or other constants) are not overwritten. Providing we cannot declare constants in python I would strongly advocate for a naming convention that prevents interference with users scripts.
I would be an error to have both lines as you mention but this can happen and we should make sure that we are robust (as much as possible) against this.

[swhite2401]

Concerning periodicity, although there are no real practical applications I understand it can be useful in the specific (academic?) case you describe. My problem is that this adds major complications throughout the whole code (division and multiplications all over the place but also missing in some other places...) for very little added value. In fact my feeling is that it is not really functional in its present state and that the usage of this functionality is really minor.
I would very much prefer that it is well contained in dedicated functions to facilitate maintenance and developments.

That being said I will not insist on removing (or changing) it if you like it but please in the help/documentation write ring.periodicity instead of NCELLS because this is really misleading.

I may propose a simpler alternative for pyAT.

[lfarv]

What you have at the moment is fine, still I think your implementation is not safe and cannot guaranty that clight (or other constants) are not overwritten.

You are right, but I think it's not worse than:

from math import pi
pi=3.14

for those who consider 3 digits is enough, or

from scipy.constants import c
c=2.998e8

which I think is even worse. math and scipy.constants are not safe either: no constants in python.

[swhite2401]

Sure none of these are good but we may try to make it a bit safer by naming our constants _clight for instance. It costs nothing and there is less probability that the internal AT constants are overwritten by mistake ( clight is obvious but what about mu0 or eps0 for example?).

I also found this (I haven't tried myself), but this could be a solution:

class _const:
    class ConstError(TypeError): pass
    def __setattr__(self,name,value):
        if self.__dict__.has_key(name):
            raise self.ConstError, "Can't rebind const(%s)"%name
        self.__dict__[name]=value
import sys
sys.modules[__name__]=_const()

[MJGaughran]

I have to admit I don't quite understand the problem with clight.

Even when you're doing something you probably should avoid, such as from at import *, I don't think setting clight actually affects the variable in the module. I may have missed the problem you're seeing, though.

If I'm wrong on that, the suggestion of including only the constants module itself in lattice would be preferable:

from at.constants import clight
from at.lattice.constants import clight

The variables should be kept as clear and simple as possible.

[lfarv]

@MJGaughran : if you really do from at import *, which his crazy, you will indeed change the value in the module.
And yes, including the constants module itself in lattice prevents from the problem. Also in the case of from at.lattice import *
@swhite2401: _clight conventionally means private variable: it should not be used. I also looked at various ways to have real constants, but all are exaggeratedly complicated.

So:

• We keep variables clear and simple, that's also the most important for me,
• We ensure a minimum protection against crazy things by not putting all the constants in the at.lattice namespace. If you explicitly import a constant, you know what you are doing.
• We will not be more picky than the python library itself and Scipy ("Pas plus royaliste que le roi")

[lfarv]

please in the help/documentation write ring.periodicity instead of NCELLS because this is really misleading.

Done, unless I missed one…

[swhite2401]

Ok fine by me, I have no royalist pretentions! And thanks for updating the help

[MJGaughran]

@lfarv I hope you don't mind, but your comment did intrigue me. I just tried the following snippet, and it does not change the value of get_rf_frequency() on your branch:

from at import *
clight = 0.0
ring = load_mat('test_matlab/hmba.mat')

ring.set_cavity(Frequency=Frf.NOMINAL)
ring.get_rf_frequency()

Changing the variable directly in constants.py will set the result to zero. This is expected from how I understand import statements to work. If you find that the above is having an effect on certain calculations or user code, something may not be configured correctly.

I think keeping constants outside of the at namespace is a reasonable design decision regardless.

[lfarv]

@MJGaughran, @swhite2401: I did another test, showing clearly what's happening:

Python 3.9.7 (default, Sep  3 2021, 12:37:55) 
[Clang 12.0.5 (clang-1205.0.22.9)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import at
>>> from at import clight
>>> clight is at.clight
True
>>> clight=0
>>> clight
0
>>> at.clight
299792458.0
>>> clight is at.clight
False
>>> 

I don't fully understand what's happening, it looks like importing makes a local copy of the module value. Reassigning the local variable breaks the link. Anyway, this shows that we are completely safe, that's nice.

To be complete, typing:
>>> at.clight=0
really breaks things ! Don't do that ! And again, it's similar with math.pi=2, this way you re-invent mathematics.

[swhite2401]

Perfect!

[lfarv]

I think keeping constants outside of the at namespace is a reasonable design decision regardless.

This is done. I think we are now ready to merge. Any other comment ?

[swhite2401]

Ok for me