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 attribute names in Simple Ring #655

Merged
merged 5 commits into from
Sep 20, 2023
Merged

Fix attribute names in Simple Ring #655

merged 5 commits into from
Sep 20, 2023

Conversation

lcarver
Copy link
Contributor

@lcarver lcarver commented Sep 15, 2023

This PR addresses the main comments from @lfarv in #647 . However I would like to discuss a bit the other comments.

I don't feel that there is any added value allowing a twiss_in input for simple_ring. In order to generate a twiss_in style input, you need a ring to begin with. In this case it is much easier to use fast_ring to generate your new ring and not bother using simple_ring.

I cannot really envisage a scenario where I might have only the output of some tracking results (a sigma matrix) and then I have to find an equivalent lattice to give me these parameters (where I still have to provide tunes and other non-linear parameters).

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to generate a twiss_in style input, you need a ring to begin with.

Why? It's just a generalised way of defining the optics properties: you give all the parameters you want in a single dict object. And since the R matrices are a generalisation of the 2D alpha and beta parameters, you can even define a fully coupled beam.

I cannot really envisage a scenario where I might have only the output of some tracking results (a sigma matrix)...

Same remark: a sigma matrix has nothing to do with tracking. It's the simplest way to describe the beam size and divergence you want (6D ellipsoid), without having to care about beta values.

Anyway, all that is not needed, my concern was about names, so it's OK for merging.
(I did not think of changing the names of the C variables, but why not…)

I'll look at the Matlab version.

pyat/at/lattice/elements.py Outdated Show resolved Hide resolved
pyat/at/lattice/elements.py Outdated Show resolved Hide resolved
@lcarver
Copy link
Contributor Author

lcarver commented Sep 20, 2023

Thanks for your comments. I made the changes you suggest. I agree we can do the simple_ring modifications with twiss in and sigma_matrix at a later date.

Ready for merge, just needs reapproval.
I will move onto your matlab branch now.
Cheers,

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All OK for me!

@lcarver lcarver merged commit 8a7101e into master Sep 20, 2023
31 checks passed
@lcarver lcarver deleted the fix_simple_ring branch September 20, 2023 13:27
lnadolski added a commit that referenced this pull request Oct 25, 2023
# By Laurent Farvacque (14) and others
# Via GitHub
* master: (28 commits)
  Add passive beamloading cavity (#586)
  Create BndStrMPoleSymplectic4RadPass (#665)
  Documentation fixes (#669)
  Update of the build process (#659)
  New Matlab function atsimplering (#657)
  Collective bugfix (#664)
  Correct the attribute name of solenoids in Matlab (#663)
  Error parsing args for twiss_in and r_4d (#662)
  Fix atmaincavities (#656)
  Fix attribute names in Simple Ring (#655)
  Remove collective passes from internal lattice_pass (#650)
  The DPStep keyword in linopt6 raises an error for 4D lattices (#651)
  Bug fix in atdisable_6d: keep the Energy field in cavities. (#654)
  fix: ring phase advances in computeRDT.m (#652)
  Correct the axis definition in plot_sigma (#648)
  Don't automatically cache the location of RF cavities (#640)
  Simple ring model (#643)
  Correct Dipole tapering (#623)
  Chromatic functions extended (#644)
  Repair the Matlab tests (#645)
  ...

# Conflicts:
#	atmat/Contents.m
#	atmat/atphysics/Radiation/atdisable_6d.m
#	atmat/atphysics/Radiation/atenable_6d.m
#	atmat/lattice/at2str.m
#	atmat/pubtools/create_elems/atidtable_dat.m
#	pyat/at/lattice/elements.py
#	pyat/at/lattice/lattice_object.py
#	pyat/at/physics/matrix.py
#	pyat/at/physics/radiation.py
#	pyat/examples/CollectiveEffects/RobinsonInstability.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants