Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug in frequency map when tune above 0.5 #745

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

oscarxblanco
Copy link
Contributor

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
Copy link
Contributor Author

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

@swhite2401
Copy link
Contributor

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
Copy link
Contributor Author

@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 ?

@oscarxblanco oscarxblanco added the WIP work in progress label Mar 6, 2024
@lfarv
Copy link
Contributor

lfarv commented Mar 9, 2024

@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 oscarxblanco removed the WIP work in progress label Mar 11, 2024
@oscarxblanco
Copy link
Contributor Author

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
Copy link
Contributor

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

@oscarxblanco
Copy link
Contributor Author

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

@oscarxblanco oscarxblanco changed the title fix bug in tune above 0.5 fix bug in frequency map when tune above 0.5 Apr 10, 2024
@oscarxblanco oscarxblanco added Python For python AT code bug fix labels Apr 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug fix Python For python AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants