rf_functions package overhaul

[alasheen]

Following issue #15

The functions require full documentation, unit-tests, exceptions handling, and to agree on the expected input/outputs of the functions.

This issue is opened to gather requests, suggestions, follow-ups.

[scpalbright]

• Raise exception rather than return empty list when no bucket found
• Make returning inner buckets a non-default option

[scpalbright]

• In rf_potential_generation if left_time == 0 the result is computed with no left margin, make margin a function of t_rev, rather than the value of left_time and right_time?

[scpalbright]

• In rf_voltage_generation I think it's a bit risky to hard code the default of t_rev / harmonic_number[0] for setting the time window. It's not very transparent and will cause issues if there's any phase offset or the harmonic numbers are not passed in the "right" order.

[alasheen]

• Auto evaluation of the number of potential well in find_potential_wells_cubic based on the harmonic numbers, the option "mest" should be renamed and adapted

[alasheen]

We need to agree on the layout of the rf_functions package. Here is the present list of functions and their input/output:

• [ ] RF voltage generation (analytical)

rf_voltage_analytic(time_array, voltage, harmonic_number, phi_offset)

return time_array, voltage_array

• [ ] RF potential generation (analytical)

rf_potential_analytic(time_array, voltage, harmonic_number,
                            phi_offset, eta_0, charge, energy_increment)

return time_array, potential_well

• [ ] RF potential generation (cubic spline intergration)

voltage_to_potential(time_array, voltage_array, eta_0, charge,
                                  t_rev, energy_increment, method='cubic',
                                  interpolated_voltage_minus_increment=None)

return time_array, potential_well, (voltage_minus_increment,
                                        interpolated_voltage_minus_increment)

• [ ] Find potential wells, using cubic spline interpolation

find_potential_wells_cubic(time_array, potential_well,
                               precision=1e-6, n_max_guess=10,
                               edge_is_max=False, verbose=False)

return (potwell_max_locs, potwell_max_vals,
            potwell_inner_max, potwell_min_locs,
            potwell_min_vals)

• [ ] Cut potential wells based on cubic spline interpolation

potential_well_cut_cubic(time_array, potential_well,
                             potwell_max_locs)

return time_array_cut_list, potential_well_cut_list

• [ ] Get the trajectory in phase space for a given potential well (cubic spline interpolation)

trajectory_area_cubic(time_array, potential_array, eta_0, beta_rel,
                          tot_energy, min_potential_well=None)

return time_array, dEtraj, hamiltonian, calc_area, half_energy_height, \
        full_length_time

• [ ] Get all the trajectories in phase space for all amplitudes in a given potential well (cubic spline interpolation)

area_vs_hamiltonian_cubic(time_array, potential_array, eta_0, beta_rel,
                              tot_energy, min_potential_well=None,
                              inner_max_potential_well=None,
                              n_points_reinterp=None)

return time_array, \
        hamiltonian_scan, calc_area_scan, \
        half_energy_height_scan, \
        full_length_time_scan

• [ ] Get the synchrotron frequency (cubic spline integration/derivative)

synchrotron_frequency_cubic(time_array, potential_array, eta_0, beta_rel,
                                tot_energy, min_potential_well=None,
                                inner_max_potential_well=None,
                                n_points_reinterp=None)

return time_array_fs, \
        sync_freq, \
        hamiltonian_scan_fs, \
        calc_area_scan_fs, \
        half_energy_height_scan_fs, \
        full_length_time_scan_fs

Please comment on the changes you would like (function name change, changes in the default input/output, changes in the expected options), and on the missing functions to be added or present in other branches/repos.

[MarkusSchwarz1980]

• I suggest to append '_analytic' to the the rf_voltage/potential_generation function to make clear how it computes these quantities. Also, replace '_cubic' by '_numeric', or is there also a '_linear' version of these functions?

AL: I agree, I included function renaming in the comment. Indeed a _linear version is expected. However I think it could/should be handled by the same function, with a "method" option.

• The input for the 'analytic' functions is unintuitive. Why not use 'time_array' as an argument, as is done with the '_cubic' versions?

AL: The first implementation was thought to compute the function between [0, t_rf], or to values defined by time_bounds if needed. I agree that the input is not intuitive. This also goes along with the comments by @scpalbright in https://github.com/blond-org/blond_common/issues/17#issuecomment-522028830 and https://github.com/blond-org/blond_common/issues/17#issuecomment-522947333 . I put time_array as a mandatory input till we get more input from others.

• Have rf_potential_generation and rf_potential_generation_cubic both return a tuple of length 3 (e.g. rf_potential_generation should return time_array, potential_well, None)

AL: More generally, do we want time_array to be returned by these functions, knowing that it is a mandatory input for all these functions ? Do we also want to ensure that all share the same ouput format ?

• Why do some function arguments have a '_full' appendix? Is this necessary?

AL: the reason is my compulsive disorder for long variable names :) . In fact, this is to make the distinction between the full potential well and the potential well cut at the location of the separatrices. To make the input uniform, I changed the output of potential_well_cut_cubic

• Why does synchrotron_frequency_cubic return a time_array_fs[sorted] array? As a user, I would expect that when I input 'synchrotron_frequency_cubic(my_time_array)' I get fs at my_time_array?

AL: this is intended. The function computes fs on the left and right hand side of the minimum, and sorts the time_array_fs in the same order as the input time_array.

• Concerning the meaning of 'frequency': To use f or omega, that is the question: 'tis nobler in the mind to suffer the slings and arrows of [a different return type of each functions], or take arms against a sea of [prefactors], and by [organizing them in a global parameter], [have an identical output for all functions]? (My apologies to Shakespeare). But my idea is to have a parameter in BLonDrc_params that governs whether functions use f or omega (e.g. Z(f) or Z(omega)).

AL: in BLonD the convention is we call f for frequency and omega for angular frequency, then the input is often frequency (e.g. for impedance sources). I am interested in the idea to be able to have this governed by the rc object, but I think this will be for an other larger Github issue :)

• Would it make sense to include a version of these functions in the Ring and RF_station classes, so that the user can simply do RF_station.synchrotron_frequency(time), and RF_station calls rf_functions.potentials.synchrotron_frequency with its parameters for eta0 etc.?

AL: this is something under discussion. We can certainly use these functions to replace the one used in the bunch generation routines (this was the original intention). However for the calculation of phi_s, f_s etc, we need to find a reasonable compromise to not include to much unecessary complexity in the tracking code (e.g. present implementation of calc_phi_s is limited, but flawless for single rf cases)

[alasheen]

• Included @MarkusSchwarz1980 's suggestions in https://github.com/blond-org/blond_common/issues/17#issuecomment-524322891

• Changed mest option to n_max_guess in find_potential_wells_cubic regarding https://github.com/blond-org/blond_common/issues/17#issuecomment-522954211

[alasheen]

Added edge_is_max option to find_potential_wells_cubic following https://github.com/blond-org/blond_common/issues/34

[scpalbright]

Set mest to a default large value in find_potential_wells_cubic