-
Notifications
You must be signed in to change notification settings - Fork 32
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
Lifetime and DA calculations for pyAT #351
Conversation
I convert this to draft since their might be many further iterations before converging to the final version |
@swhite2401: works nicely as expected, but there is a problem: Instead of your awful refpts combination: ids = ring.get_refpts('ID*')
quads = ring.get_refpts(at.Quadrupole)
dips = ring.get_refpts(at.Dipole)
ncells = 1
refpts = numpy.sort(numpy.concatenate((ids[:ncells],
quads[quads<ids[ncells]],
dips[dips<ids[ncells]]))) I'm using a simpler combination with boolean refpts: ncells = 1
ids = ring.get_cells(at.checkname('ID*'))
quads = ring.get_cells(at.checktype(at.Quadrupole))
dips = ring.get_cells(at.checktype(at.Dipole))
# Combine references
refpts= ids | quads | dips
# Set cells > ncells to False
cellstart = ring.uint32_refpts(ids)
refpts[cellstart[ncells]:] = False Using booleans is usually more efficient, but At least at line 98 of But there may be other problems… |
class GridMode(Enum):
""""
Class to defined the grid mode use when searching
for the boundary
"""
RADIAL = 0
GRID = 1
RECURSIVE = 2 CARTESIAN instead of GRID ? |
By analogy with
It makes things simpler:
def set_ring_orbit(ring, dp, refpts, orbit):
"""
Returns a ring starting at refpts and initial
closed orbit
"""
if refpts is not None:
assert numpy.isscalar(refpts), 'Scalar value needed for refpts'
newring = newring.rotate(refpts)
if orbit is None:
orbit, _ = ring.find_orbit(dp=dp)
return orbit, newring Now, if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fine for me (once it passes all the checks)
Latest improvements: |
@lfarv I had to exclude again the python 3.7 windows test but I saw it was commented on the master...how do you want to handle this? |
@swhite2401 : the master branch has modifications in at.c and some .py files to allow the tests again. For the moment, take the In the future, I would like to remove the workarounds in the pyat code a keep a clean version. With that, the test failed for Windows/3.7 in the old Github configuration, and would fail for Windows/3.6 in the present one (so exclusion in the test sequence). But the AT code would be clean. |
Ok restore |
All the tests are now running ? very strange… Apparently GitHub changed again. And maybe corrected the bugs. I'll look at that. Anyway it looks ok for merging. |
@swhite2401 : now I understand: you merged the changes in master, which allows the tests to run. But the problem is still there, like for instance in the |
Final verification regarding lifetime calculation for @simoneliuzzo . Then I compute the lifetime using the default method in matlab, only one method is provided in python, results are: Python MA + integral: 36.41h For me this is good, small difference in MA can be explained by difference in the algorithm and default tolerances. To be noted: I have corrected the python to match the matlab but I am not convinced this is correct, naively I would have thought that the distance between elements makes more sense as it implicitly assume constant MA between elements. Adding elements would refine this approximation. @simoneliuzzo , @carmignani , @lfarv you insight would be much appreciated on this. Besides that I think all functionalities are validated and this is ready for the merge. |
Ah yes you are right! I made a mess at some point...sorry for the confusion |
These are equivalent if However, the MA and beam dimensions are computed at the entrance of the selected element, and the values are applied over the following length. Wouldn't it be better to compute the weight (or length) as going from half-way from the previous element to half-way to the new element? |
Yes you are right, this would even be better! (selection of zero length elements is properly discarded in matlab, I have not done it because it was not necessary with my initial idea...) So how would you like to proceed? This has consequences for the matlab that everyone is using...and from the test I have done results in underestimated lifetime. This certainly goes beyond this PR. |
Concerning the possible improvement, up to you to see if it's worth going to that in another PR. |
I am clearly more comfortable with python, but I can give it a try for matlab (on a separate PR) |
Ok so discussing with @carmignani we thought of a better solution that is to use the optics calculated at each element + interpolated momentum aperture (MA calculation is the time consuming part) and distance between interpolation points. This is what we think would be the less approximated solution. I will make a proposal for the python in this direction. |
Final check on the DA, using either the interpolated method discussed above or the previous per element method from matlab. Each points represents different sampling rate (1 all elements, other every n element). Both method give identical results when all elements are used, for fewer elements the interpolated method fluctuates less. For me this is ready to merged. Any objection? |
This branch allows the compute the dynamic aperture and lifetime of a pyAT lattice.
Below a script with example usage.
To be noted:
use_mp=True
has crashed in the past with MacOS, to be tested again with the newpatpass
#347TODO:
1-
different radii distributionDone2-
divider in recursive search (defaults to 2 now)Done3-
search for max range in 2D -> use as starting pointTested: no performance gain4-cluster submission (separate PR)