-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: master
Are you sure you want to change the base?
Conversation
@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() |
@lfarv I need advice here: the Now I am going to complain sorry...: this My perference is to remove it off course... now this is up for discussion... |
Dear @swhite2401 , 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. |
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). The only useful case I could identify is for Anyway, this is my users and developers opinion not necessarily the one of the majority... |
This PR was initially answering #836, however by doing so I realized that the function
get_tune()
andchromaticity()
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?