Fitting Base Class and Gaussian Model from PR 143 #172

eloiseyang · 2024-06-12T23:25:46Z

Added MethodBase as an abstract class for all fit methods.
Added GaussianModel class using MethodBase.
Added ProjectionFit class.
Added tests for all the above including datasets.
Separated from pull request 143.

Added GaussianModel class using MethodBase Added ProjectionFit class Added tests for all the above including datasets Separated from 8c85a56 on phys-cgarnier/lcls-tools/dev

eloiseyang

These fitting classes were written with ML in mind. Can we find the intersection between the ML and physics needs? Otherwise, we may have to separate out the ML fitting tools and the physics fitting tools.

eloiseyang · 2024-06-26T22:02:33Z

tests/unit_tests/lcls_tools/common/data_analysis/fit/test_projection.py

We should complete this before merging.

lcls_tools/common/data_analysis/fit/methods.py

eloiseyang · 2024-06-26T22:31:36Z

lcls_tools/common/data_analysis/fit/methods.py

+        weighted_sigma = np.sqrt(np.cov(x, aweights=data))
+
+        self.init_values = {
+            self.param_names[0]: weighted_mean,  # data.mean()


The text keys are more clear than the numbered indices here. I don't imagine these param names are going to change all that often. Maybe a pydantic structure would be a better way of accomplishing what this does?

I agree. We can discuss what that would look like in our meeting.

eloiseyang · 2024-06-26T22:31:58Z

lcls_tools/common/data_analysis/fit/methods.py

+        # Creating a normal distribution of points around initial offset.
+        offset_prior = norm(self.init_values["offset"], 0.5)
+        self.priors = {
+            self.param_names[0]: mean_prior,


Numbered indices are not clear.

I believe Nicole did not want hardcoded names though, so the compromise was we made a class attribute than contained the param names as a list that wouldn't get overwritten easily. I agree the using indices on that list might be confusing to a reviewer though, I am not sure what the best way to resolve both problems is.

My understanding is that we want to avoid hard coded EPICS names. Hard coded keys of a predefined dictionary should be okay. I think defining a struct-like class could be a good solution here.

eloiseyang · 2024-06-26T22:32:48Z

lcls_tools/common/data_analysis/fit/methods.py

+    def _forward(self, x: np.array, params: np.array):
+        # Load distribution parameters
+        # needs to be array for scipy.minimize
+        mean = params[0]


Numbered indices are not clear. What is the params list?

Params list is the list of parameters the fit function uses. At the method class level, they need to be reference arbitrarily Furthermore, scipy.optimize.minimize requires they be stored in an ordered list.

eloiseyang · 2024-06-26T22:54:14Z

lcls_tools/common/data_analysis/fit/projection.py

+        params_dict = self.unnormalize_model_params(fitted_params_dict, projection_data)
+        return params_dict
+
+    def plot_fit(self, **kwargs):


We should separate out plotting functions.

eloiseyang · 2024-06-26T22:54:31Z

tests/unit_tests/lcls_tools/common/data_analysis/fit/test_methods.py

+        self.assertAlmostEqual(fit.min(), 0.0, places=self.decimals)
+        return
+
+    # def super_gaussian(self, x, amp, mu, sig, P, offset):


Can this be removed?

We should delete this test before we push.

eloiseyang · 2024-06-26T22:55:29Z

lcls_tools/common/data_analysis/fit/method_base.py

+        and upper bound on for acceptable values of each parameter)
+    """
+
+    param_names: list = None


This design pattern is exactly the kind of thing pydantic is meant to simplify. Can we a simpler way to accomplish this?

lcls_tools/common/data_analysis/fit/method_base.py

Fit

…lving params, might have to remove priors still but its working

…ters

made changes as requested in the comment (minus removing _forward)

Created Pydantic Fitting Parameters Class

eloiseyang · 2024-07-15T20:32:04Z

@phys-cgarnier added some changes to the way we set parameters in the model. Waiting on a single function fitting tool for physics applications before merging.

… have a default field, also set method in projection_fit.py to have a default file. This will make using the tool much easier. All that has to be down is call gaussian fit and give it an image to use

Gaussian FIt

eloiseyang · 2024-07-29T22:35:27Z

Thanks for pushing the beamsize code, @phys-cgarnier. It looks good to me, we just have some flake8 tests failing. If you could fix those I think we're ready to push this branch. We'll want to write some regression tests for the gaussian fit and beamsize scripts too, but I think that can wait for now.

@nneveu can you take a look too?

nneveu

This looks much better, thanks for the the edits @phys-cgarnier and @eloiseyang! I'd like to see a few more comments for functions, remove the commented out test, and an issue written on circling back to the naming conventions, but otherwise ok to merge. Thanks so much!

nneveu · 2024-07-30T19:13:37Z

lcls_tools/common/data_analysis/fit/gaussian_fit.py

+            }
+            return result
+        else:
+            centroid = beamspot_chars['centroid']


Can we comment the two cases here for calc beamsize, i.e. you do the first if the intensity is low and the else if intensity is high enough? and how was the default threshold picked?

I will comment both on both cases. The default was selected by the ML group, I just used theirs so I am sure how the decided on it.

nneveu · 2024-07-30T19:14:48Z

lcls_tools/common/data_analysis/fit/gaussian_fit.py

+                    "Sx": np.NaN,
+                    "Sy": np.NaN,
+                    "bb_penalty": np.NaN,
+                    "total_intensity": beamspot_chars["total_intensity"],


This is repeated except the 10**? maybe combine with above?

nneveu · 2024-07-30T19:15:20Z

lcls_tools/common/data_analysis/fit/gaussian_fit.py

+
+
+                print(pts)
+


do we want to leave this print?

No thats a typo, lol.

nneveu · 2024-07-30T19:15:44Z

lcls_tools/common/data_analysis/fit/gaussian_fit.py

+                n_stds = self.bounding_box_half_width
+
+                pts = np.array(
+                    (


can we be more descriptive than pts?

Sure, I will rename it

nneveu · 2024-07-30T19:16:21Z

lcls_tools/common/data_analysis/fit/gaussian_fit.py

+                temp = pts - np.array((roi_radius, roi_radius))
+                distances = np.linalg.norm(temp, axis=1)
+                # subtract radius to get penalty value
+                bb_penalty = np.max(distances) - roi_radius


bounding box penalty or something else? can you add a comment?

Sure I will add a comment with more description.

nneveu · 2024-07-30T21:46:18Z

lcls_tools/common/data_analysis/fit/methods.py

+        return priors
+
+    def _forward(self, x: np.array, method_parameter_list: np.array):
+        # Load distribution parameters


This is the function doing the fit right? can we rename it to something with 'fit' in the name?

maybe a #TODO comment to revisit would be enough for now.

nneveu · 2024-07-30T21:48:09Z

lcls_tools/common/data_analysis/fit/projection.py

+        return normalized_data
+
+    def unnormalize_model_params(
+        self, method_params_dict: dict, projection_data: np.ndarray


Why does 'model' need to be in this function name?

nneveu · 2024-07-30T21:54:05Z

lcls_tools/common/data_analysis/fit/projection.py

+        """sets up the model and init_values/priors"""
+        self.model.profile_data = projection_data
+
+    def fit_model(self) -> scipy.optimize._optimize.OptimizeResult:


do we want to say fit_model or fit_method here?

nneveu · 2024-07-30T21:55:00Z

lcls_tools/common/data_analysis/fit/projection.py

+    def fit_model(self) -> scipy.optimize._optimize.OptimizeResult:
+        """
+        Fits model params to distribution data and plots the fitted params
+        as a function of the model.


Are we still plotting? maybe update this comment

nneveu · 2024-07-30T21:56:39Z

tests/unit_tests/lcls_tools/common/data_analysis/fit/test_projection.py

Why the empty file?

I don't know I believe there was a test there originally. I will remove it then write the test in another PR

Added MethodBase as an abstract class for all fit methods

37567e7

Added GaussianModel class using MethodBase Added ProjectionFit class Added tests for all the above including datasets Separated from 8c85a56 on phys-cgarnier/lcls-tools/dev

eloiseyang assigned eloiseyang and nneveu Jun 12, 2024

eloiseyang requested a review from nneveu June 12, 2024 23:29

method_base.py flake8

fda0b90

eloiseyang commented Jun 26, 2024

View reviewed changes

phys-cgarnier added 3 commits July 10, 2024 10:25

Merge pull request #1 from slaclab/fit

318397a

Fit

changed code base to be more pydantic this cleans up some things invo…

640010c

…lving params, might have to remove priors still but its working

updated fit code to incorporate pydantic structures for method parame…

2a47d30

…ters

eloiseyang force-pushed the fit branch from 284c555 to fda0b90 Compare July 12, 2024 21:23

eloiseyang and others added 6 commits July 12, 2024 15:09

Merge commit 'b6949b679df756059f6edd29b83d8dd165d6d4bf' into fit

8a38fed

Merge branch 'fit' into fit_pydantic

06444bf

made changes as requested in the comment (minus removing _forward)

a34fbf9

Merge pull request #3 from phys-cgarnier/main

5d4a149

made changes as requested in the comment (minus removing _forward)

Merge pull request #2 from slaclab/fit_pydantic

387fae4

Created Pydantic Fitting Parameters Class

flake8

eb32f59

phys-cgarnier added 3 commits July 24, 2024 15:09

committing GaussianFIt with property that returns beamsize

59bf2ae

checked that GaussianFit works (roughly) and set processor and fit to…

8b23286

… have a default field, also set method in projection_fit.py to have a default file. This will make using the tool much easier. All that has to be down is call gaussian fit and give it an image to use

Merge pull request #4 from phys-cgarnier/main

8dbafca

Gaussian FIt

nneveu approved these changes Jul 30, 2024

View reviewed changes

phys-cgarnier added 5 commits July 31, 2024 14:29

committing linted gaussian_fit.py

b70e68c

removed empty test for projection

cb1cec9

Merge branch 'phys-cgarnier:main' into fit

dfe72bc

more flake8

ca54c90

Merge branch 'phys-cgarnier:main' into fit

6090968

nneveu merged commit c950303 into slaclab:main Aug 2, 2024
3 checks passed

nneveu deleted the fit branch August 2, 2024 18:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fitting Base Class and Gaussian Model from PR 143 #172

Fitting Base Class and Gaussian Model from PR 143 #172

eloiseyang commented Jun 12, 2024

eloiseyang left a comment

eloiseyang Jun 26, 2024

eloiseyang Jun 26, 2024

phys-cgarnier Jul 10, 2024

eloiseyang Jun 26, 2024

phys-cgarnier Jul 10, 2024

eloiseyang Jul 10, 2024

eloiseyang Jun 26, 2024

phys-cgarnier Jul 10, 2024

eloiseyang Jun 26, 2024

eloiseyang Jun 26, 2024

eloiseyang Jul 29, 2024

eloiseyang Jun 26, 2024

eloiseyang commented Jul 15, 2024

eloiseyang commented Jul 29, 2024

nneveu left a comment

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

nneveu Jul 30, 2024

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

nneveu Jul 30, 2024

nneveu Jul 30, 2024

nneveu Jul 30, 2024

nneveu Jul 30, 2024

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

nneveu Jul 30, 2024

phys-cgarnier Jul 31, 2024

Fitting Base Class and Gaussian Model from PR 143 #172

Fitting Base Class and Gaussian Model from PR 143 #172

Conversation

eloiseyang commented Jun 12, 2024

eloiseyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eloiseyang commented Jul 15, 2024

eloiseyang commented Jul 29, 2024

nneveu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment