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

Tune, chromaticity and detuning periodicty #837

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

Conversation

swhite2401
Copy link
Contributor

This PR was initially answering #836, however by doing so I realized that the function get_tune() and chromaticity() were also not taking into account the ring periodicity.

This is now fixed, black and import numpy as np were applied to all the modules that are modified in this PR, most of the changes come from these....

I think this is a necessary fix but is not backward compatible and may affect scripts using a periodicity different from 1, is this acceptable?

pyat/at/physics/linear.py Outdated Show resolved Hide resolved
@swhite2401
Copy link
Contributor Author

@oscarxblanco thanks for the check, I have added a unit test to check that the periodicity is correctly implemented in get_tune(), get_chrom(), detuning() and chromaticity()

@swhite2401
Copy link
Contributor Author

@lfarv I need advice here: the periodicity is also not implemented in matlab for the tunechrom function. This function may be used in many places and I am really not sure what the consequences would be to correct it.

Now I am going to complain sorry...: this periodicity parameter is presently a disaster we have no idea where it applies and where it does not and no one I know of is using it. I did put an issue in the past on this topic and it was not fixed.
So now either we completely remove it, which would simplify a lot the code or we fix it once and for all, but this hybrid situation is really very very bad an unacceptable.

My perference is to remove it off course... now this is up for discussion...

@oscarxblanco
Copy link
Contributor

Dear @swhite2401 ,
the last commit is giving me the total chromaticity and detuning as expected when including the periodicity.
About the tune, the fractional part given by get_tune matches what I expected when including the periodicity. This is OK for me.

It could be somehow unexpected that a fodo returns such a big chromaticity and tune that is far from the one that could be estimated for a single cell.

If I may give an opinion on the periodicity, I think it should be stated in the DocString when and when is not considered. And also somewhere in the documentation if periodicity is fixed or could be override or overwritten. This because it is sometimes is useful to have the values for a single fodo cell.

@swhite2401
Copy link
Contributor Author

This because it is sometimes is useful to have the values for a single fodo cell.

How to set periodicity is probably documented together with the lattice object, I have to check.... but in any case I agree that it is clearly not adequate and has to be improved

However, the feedback that I got (and that I share) is that the periodicity is an annoying feature rather than something convenient. In the users end up proceeding like this: when they want one cell they load one cell with periodicity=1 and when they want the full ring they do ring.repeat(ncells).
This is simple, well understood and you know what you are doing and what to expect. Anything else produces random results depending on what function is called or how you loaded you lattice.

The only useful case I could identify is for radiation_parameters(), envelope_parameters() and eventually to generate a fast_ring. Periodicity could be set locally in these functions rather than polluting the whole code...

Anyway, this is my users and developers opinion not necessarily the one of the majority...

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.

2 participants