fix bug in frequency map when tune above 0.5

oscarxblanco commented 4 months ago

Dear all, this PR fixes a bug.

The fractional tune returned by the frequency analysis lies between 0 and 0.5, which, mismatches the tune representation in AT (0 to 1), and would be visible in lattices with any tune above 0.5. In this PR, the fractional tune returned by the frequency analysis is corrected as 1 minus the value when the lattice tune is above 0.5.

This should solve the issues in most cases except when the tune footprint crosses 0, 0.5 or 1.

I leave here the code to do a quick test:

''' 
This file is used to test the ring tune above 0p5
oblanco 2024mar01
'''

import at
import numpy

print(f'{at.__version__}=')

QF=at.Quadrupole('QF',0.5,1.2)
Dr = at.Drift('Dr', 0.5)
HalfDr = at.Drift('Dr2', 0.25)
QD = at.Quadrupole('QD', 0.5, -1.2)
Bend = at.Dipole('Bend', 1, 2*numpy.pi/30)

ring = at.Lattice(3*[HalfDr, Bend, Dr, QF, Dr, Bend, Dr, QD, HalfDr],
                      name='Simple FODO cell', energy=1E9)

print(f'{ring.get_tune()=}')

eoffset=0
pool_size=12
turns=1024
fmadata = at.fmap_parallel_track(ring,
    coords=[-10,10,-10,10],
    add_offset6D=numpy.array([0,0,0,0,eoffset,0]),
    steps=[3,3],
    pool_size=pool_size,
    turns=turns, verbose=False);
fmadata=numpy.array(fmadata[0])

nuxlist = fmadata[:,2]
nuylist = fmadata[:,3]
print(f'{nuxlist=}')
print(f'{nuylist=}')

oscarxblanco commented 4 months ago

Dear @swhite2401, would you review this PR or would you prefer someone else to review it ?

swhite2401 commented 4 months ago

Hi @oscarxblanco , I can review it if you are a bit patient, the coming week is quite busy for me. Is it ok for you?

oscarxblanco commented 4 months ago

@swhite2401 , no problem at all. This is a minor bug. Besides I still have some checks that do not pass automatically that appeared when I merged the master into this branch. I have to check what is happening.

From experience this should be solved with a PEP8 check. Is there any PEP8 application that you suggest ? In another disscussion I read @lfarv uses black (https://black.readthedocs.io/en/stable/), right ?

lfarv commented 3 months ago

@oscarxblanco: The failure you encountered is strange: a crash on a Windows run. A new attempt to run the tests succeeded so your branch is correct.

Concerning PEP8 conformance, it's a matter of coding style, but it has no effect on the run time behaviour of the code. The test sequence does include a PEP8 conformance test (5th step called "Lint with flake8"), but it does not affect the success of the sequence. It just prints out a list of all non-conforming lines, which I think very few people look at.

Now to ensure PEP8 conformance, I'm using PyCharm for development, which permanently displays warnings about all non-conforming code. But it's then put to you to correct. And you are right, for some time now, I've been trying "black" on new or modified files. I'm quite happy with it, with the following advantages:

no need to care about formatting when writing code, "black" will take care of it,
since "black" has very little configuration, code from different users will look very similar, which is helpful. "black" makes some choices on the way code should look like, it may not be you preferred style, but it does the job,
"black" integrates nicely in PyCharm: the "Reformat Code" command can be configured to call "black",
"black" is widely used: its result will not look "exotic".

I was about to post a pull request modifying the "developer notes" of the documentation to add a recommendation on how to install and run "black" for code formatting. What do you think of that?

oscarxblanco commented 3 months ago

Dear @lfarv , I finally got to have a quick look on black, and so far it is good.

Installation is easy through pip, and running it seems simple. I tested it in a 100 lines python script and it corrects the length of the lines automatically... it also seems possible to activate or desactivate in certain regions through pragma commands.

swhite2401 commented 3 months ago

@oscarxblanco , the implementation seems fine to me. However, should we keep this feature as optional? The documentation would also need to be improved

oscarxblanco commented 2 months ago

Dear @swhite2401 , I have added a new option called nu0p5 that is False by default.

There will be a warning if any of the ring transverse tunes is above 0.5 and nu0p5 is False. For example

ring.get_tune()=array([0.81707699, 0.73766671])
Warning: at least one tune is above 0.5, and nu0p5 flag is False

The frequency map data is modified only if nu0p5 is True and any of the ring tunes is above 0.5.

Here again is a test code, it is the same as before but adding the nu0p5 flag:

''' 
This file is used to test the ring tune above 0p5
oblanco 2024mar01
'''

import at
import numpy

print(f'{at.__version__}=')

QF=at.Quadrupole('QF',0.5,1.2)
Dr = at.Drift('Dr', 0.5)
HalfDr = at.Drift('Dr2', 0.25)
QD = at.Quadrupole('QD', 0.5, -1.2)
Bend = at.Dipole('Bend', 1, 2*numpy.pi/30)

ring = at.Lattice(3*[HalfDr, Bend, Dr, QF, Dr, Bend, Dr, QD, HalfDr],
                      name='Simple FODO cell', energy=1E9)

print(f'{ring.get_tune()=}')

eoffset=0
pool_size=12
turns=1024
fmadata = at.fmap_parallel_track(ring,
    coords=[-10,10,-10,10],
    add_offset6D=numpy.array([0,0,0,0,eoffset,0]),
    steps=[3,3],
    pool_size=pool_size,
    turns=turns,
    verbose=False,
    nu0p5=True);
fmadata=numpy.array(fmadata[0])

nuxlist = fmadata[:,2]
nuylist = fmadata[:,3]
print(f'{nuxlist=}')
print(f'{nuylist=}')

@swhite and @lfarv , do you consider mandatory to execute black on this script ? It will add modifications that I don't know if you suggest to include now.

Best regards, o

atcollab / at

fix bug in frequency map when tune above 0.5 #745