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

fitting package overhaul #26

Open
alasheen opened this issue Sep 12, 2019 · 8 comments
Open

fitting package overhaul #26

alasheen opened this issue Sep 12, 2019 · 8 comments
Assignees
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@alasheen
Copy link
Collaborator

Following issue #25

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.

@alasheen alasheen added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 12, 2019
@alasheen alasheen self-assigned this Sep 12, 2019
@alasheen
Copy link
Collaborator Author

alasheen commented Sep 12, 2019

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

  • FWHM (converters could be defined in the **options)
FWHM(time_array, data_array, **options):

return maximum, center, fwhm
  • RMS
RMS(time_array, data_array, **options):

return mean, rms
  • Peak value (at 100% of the max or averaged at lower %)
peak_value(time_array, data_array, **options):

return position, amplitude
  • Integrated profile
integrated_profile(time_array, data_array, **options):

return integrated_value
  • Calculated bunch parameters assuming a binomial distribution and obtained from the width at different maxima ratios
binomial_from_width_ratio(time_array, data_array, **options):

return fitted_parameters
parabolic_line_fit(time_array, data_array, **options)

return fitted_parameters
  • Base Distribution objects/functions
parab_line_instance = ParabolicLine(time_array, *parameters, **options) (a fit could be directly performed by passing a data_array instead of parameters)

parab_line_instance.full_bunch_length
parab_line_instance.rms
parab_line_instance.rms4
parab_line_instance.fwhm

parab_line_instance.profile(time_array, *parameters) <- generate a profile, we should store the result to avoid computing every time the profile (decorators? if time_array != self.time_array)
parab_line_instance.time_array

parab_line_instance.spectrum(freq_array, *parameters)  <- generate a spectrum, we should store the result to avoid computing every time the spectrum (decorators? if freq_array != self.freq_array)
parab_line_instance.freq_array

parab_line_instance.fit(time_array, data_array, **options) <- inherit from base fitting function
parab_line_instance.fit_time_array
parab_line_instance.fit_data_array
...

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
Copy link
Collaborator

MarkusSchwarz1980 commented Sep 13, 2019

The Distributions objects should also have four_sigma_FWHM since this is a common length used in operation (and maybe four_sigma_RMS). Besides a Distribution.profile() function, a Distribution.spectrum() would also be useful.

AL:
- Added rms4 to the list above as well as profile/spectrum. We will need to have an implementation of profile/spectrum that would allow to return those directly if available rather than recalculating it at every call for efficiency.
- Concerning four_sigma_FWHM, the rms/fwhm values can be computed directly analytically from the distribution parameters so it is not necessary to pass from one another. To get the four_sigma_FWHM from a measured profile, without passing by the fitted parameters, I would use directly by the FWHM function with a "conversion" argument in the options, to avoid mixing too many definitions.

@MarkusSchwarz1980
Copy link
Collaborator

MarkusSchwarz1980 commented Sep 16, 2019

Maybe this should be a separate issue, but while we are at it: I also opt for the integ and diff functions of the math.calculus to also return only the quantity of interest, say Y and not a tuple of x, Y.

AL: Yes we'll also open a new issue for the math package to discuss input/output. In that case the reason to output (x, Y) is that for the diff function the ouput x array is different from the input (1 element shorter). For me either way is fine but I'd keep the same conventions for all functions in the end.

@alasheen
Copy link
Collaborator Author

Updated #26 (comment) with @MarkusSchwarz1980 comments

@alasheen
Copy link
Collaborator Author

@alasheen
Copy link
Collaborator Author

The fitting.profile module clean up was completed and merged with #27

@giuliapapotti
Copy link

I would love to have the bunch peak returned together with the bunch length and position in the fitting functions, as it is one of the standard measurables of RF beam diagnostics. In particular, for the fwhm and Gaussian fits, it is already calculated anyway (the max used for the "half max" in the fwhm, the fit amplitude for the Gaussian fit), so it is just a matter of exposing it as a separate output parameter.

scpalbright added a commit that referenced this issue Mar 20, 2020
updated CI configs, testing py36 and py37
@alasheen
Copy link
Collaborator Author

Added maximum as output for FWHM function to answer for @giuliapapotti in #26 (comment) in pull request #41

For the profile fitting functions, the amplitude should already be returned. All should follow amplitude, position, length, exponent (if applicable) as output of the fitting function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants