Creating a new fitting toolkit and examples of how to use it with Screen Profiles. #143

phys-cgarnier · 2024-04-11T17:23:56Z

Updating fitting tools, image processing, and screen beam profile measurement classes, draft PR

…rojection_fit and roi), also updated image_processing.py

…_measurements.py

…plan is to build out screen_beam_profile and make test notebook a unit test.

…dded to the test and example for screen_beam_profile_measurement.py

…o dev

…nce there is a conflict with python 3.8

nneveu · 2024-04-16T18:41:50Z

Thanks for all your hard work on this! Some initial comments after looking at a few files:

There are a lot of changes here, probably 2-3 PR's worth. I recommend submitting PR's with less changes. I.e. some of the image changes could have been in their own PR vs. being added with the fitting work.
I think plotting should be separate from more fundamental functions like fitting/image processing.
Please rename the PR to something a little more descriptive than Dev, this will help us if we want to look back at the PR later. I suggest relating it to an issue # you are trying to close.

Thanks again, great work!

nneveu

Same comments as above. I'll look more carefully at the measurement and test files after we chat.

nneveu · 2024-04-16T17:42:42Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+
+    @abstractmethod
+    def find_init_values(self, data: np.ndarray) -> list:
+        pass


Change all 'pass'es to NotImplementedError.

Even for abstract methods?

I think we want to make sure no one calls this w/o using something that overwrites the behavior? So there should be a catch for users that call this but it does not behave how they expect? I haven't used these a lot though, so maybe it's fine, we can discuss at the meeting.

nneveu · 2024-04-16T17:43:42Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+
+    def plot_init_values(self):
+        """Plots init values as a function of forward and visually compares it to the initial distribution"""
+        fig, axs = plt.subplots(1, 1, figsize=(10, 5))


I'm not a fan of hard coding figure subplots and sizes. Can we either rework this to be more general, or remove the plotting part as a function somewhere else?

nneveu · 2024-04-16T17:44:38Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+    def plot_priors(self):
+        """Plots prior distributions for each param in param_names"""
+        num_plots = len(self.priors)
+        fig, axs = plt.subplots(num_plots, 1, figsize=(10, 10))


Same here with hard coding fig size etc. idk if this belongs in a base model?

nneveu · 2024-04-16T17:51:35Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+        pass
+
+    def log_likelihood(self, x, y, params):
+        return -np.sum((y - self.forward(x, params)) ** 2)


This uses a function that is not implemented? Can we get this from scipy instead of calc ourselves? Something like: https://stackoverflow.com/questions/59869759/log-likelihood-function-generated-by-scipy-stats-rv-continuous-fit ?

I will look into this.

nneveu · 2024-04-16T17:52:40Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+        loss_temp = -self.log_likelihood(x, y, params)
+        if use_priors:
+            loss_temp = loss_temp - self.log_prior(params)
+        return loss_temp


Are these functions for ML related calcs? Loss/log liklihood/priors, etc?
If so, maybe the belong in an ML specific file

Yes these functions are essentially what we want to minimize, the minimization tells us the best fit parameters. In the case where log prior is used our minimization problem turns into a bayesian regression problem.

technically, the use of priors/Bayesian inference isn't really ML and doesn't have that much overhead, I'm not sure if we should separate into different classes. We should discuss

nneveu · 2024-04-16T18:50:01Z

tests/unit_tests/lcls_tools/common/image_processing/test_image_processing.py

The matlab2py file still exist, so this file should still exist too. May need to get updated or renamed, but the functions being tested here are still in the repo? Unless I'm missing something?

nneveu · 2024-04-16T18:59:05Z

lcls_tools/common/image_processing/roi.py

+
+class ROI(BaseModel, ABC):
+    roi_type: str
+    center: List[PositiveFloat]


I see from the indexing below that center is two floats (which makes sense x/y), maybe show that shape here or comment. Should we force this to always be a list of two floats?

reminder ask ryan

Is this the ROI for an image? If so, should these be integers so you get the centre x/y pixel? or if it's in millimeters I would add the units to the variable name

nneveu · 2024-04-16T19:06:31Z

lcls_tools/common/measurements/screen_beam_profile_measurement.py

note to self: look at this file again after next commits.

nneveu · 2024-04-16T19:06:49Z

tests/unit_tests/lcls_tools/common/measurements/test_screen_beam_profile.py

note to self: look at this file again after next commits.

nneveu · 2024-04-16T19:08:04Z

lcls_tools/common/image_processing/roi.py

Will there be a test for these?

…oi and image processor

…quite done

…sing and test_roi

…ile_data, working through request to change self.init_values in methodBase from a list a dictionary

phys-cgarnier · 2024-04-30T18:55:42Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+        self.param_names: list = None     
+        self.param_bounds: np.ndarray = None
+        self.init_values: dict = None
+        self.init_values_list: list = None


Recall that projection_fit.py needs a list in order to use fit_model. If you change the type of self.init_values to a dict and unpack it to an ordered list inside fit model then now ProjectionFit becomes model dependent instead of a template. (knowledge of the gaussian model params should not be in projection_fit). Recommending instead make a dictionary and a list of param values inside MethodBase.

see my other comment on why init_values_list is not needed

phys-cgarnier · 2024-04-30T20:58:10Z

lcls_tools/common/data_analysis/projection_fit/method_base.py

+        if not isinstance(profile_data, np.ndarray):
+            raise TypeError("Input must be ndarray")
+        self._profile_data = profile_data
+        self.find_init_values(self._profile_data)


Changed function call here from self.find_priors to self.find_init_values. Please see the follow comment for changes in self.find_init_values

phys-cgarnier · 2024-04-30T21:01:44Z

lcls_tools/common/data_analysis/projection_fit/gaussian_model.py

+        mean = np.argmax(gaussian_filter(data, sigma=5)) / (len(data))
+        sigma = 0.1
+        self.init_values_list = np.array([amplitude, mean, sigma, offset])
+        self.init_values = {"amplitude":amplitude,"mean":mean,"sigma":sigma,"offset":offset}


it was requested that users might like the option to grab init values and not have to have knowledge of what parameters belonged to the ordered np.array. So self.init_values was changed from a list to a dictionary. However, to run scipy.optimize.minimize an np.array is still needed so that is saved in a different variable.

we shouldn't save it in a different variable, we should just do the mapping inside the scipy.optimize.minimize call, should be a one-liner

I apologize, I did nor realize python dictionaries are ordered.

while technically dicts are ordered we don't have to rely on that, we can use
np.array([self.init_values[name] for name in self.param_names])

phys-cgarnier · 2024-04-30T21:17:48Z

lcls_tools/common/data_analysis/projection_fit/gaussian_model.py

+        self.init_values_list = np.array([amplitude, mean, sigma, offset])
+        self.init_values = {"amplitude":amplitude,"mean":mean,"sigma":sigma,"offset":offset}
+        # if use_priors = True in projection_fit then find priors? use case where projection fit is instantiated with use_priors = False then flag is changed but you have no priors
+        self.find_priors()


I highly recommend calling self.find_priors when init_values are found.

Lets consider the workflow for projection_fit.fit_projection(some_projection_data).
1.) This method passes normalized projection data to projection.setup_model.
2.) projection.setup_model calls the setter method for gaussian_model.profile_data
3.) The setter method for gaussian_model.profile_data triggers self.find_init_values
4.) those init values are used in projection_fit.fit_model()
5.) now consider the case when projection_fit.use_priors = True, every thing works smoothly right? Well no.
6.) projection_fit.use_priors = True can be set upon initialization or after. Also, the advantage of having gaussian_model.profile_data be a @Property is that it can be updated easily. Meaning that we can call projection_fit.fit_projection( with_any_1d_array).
7.) So use_priors = True but priors have not changed. Which is not good. Calling gauss_model.find_priors inside
projection_fit.fit_model() is also bad.

edit: I am not sure why stuff is bolded.

phys-cgarnier · 2024-04-30T22:01:54Z

lcls_tools/common/data_analysis/projection_fit/projection_fit.py

+        #making an ordered list here is making this fit_model model dependent since it needs knowledge of the params list ordering
+
+        res = scipy.optimize.minimize(
+            self.model.loss,


I think a see a potential problem with making forward and log prior receive a dictionary. Model.loss calls forward and log prior. The problem is that it is scipy.optimize.minimize would pass an unpacked list of length == len(model.init_values) to model.loss on reiterations.
I can't think of how I would be able to resolve this.

change the calls in model.loss to model._forward and model._log_prior which take np arrays instead. The public versions can take dicts as arguments

… to working code on main

… will be used in merge

…d maybe make norm a private variable of gaussModel.py and stop using _forward as a staticmethod

…iob pydantic fields.

… is called

…on to fit without displaying normalized values, changed fit_model method in scipy.optimize.minimize from default (BFGS) to Powell

nneveu · 2024-08-02T20:51:31Z

@phys-cgarnier @eloiseyang should we close this PR now that it's been reworked/merged elsewhere?

eloiseyang · 2024-08-02T20:56:36Z

@phys-cgarnier @eloiseyang should we close this PR now that it's been reworked/merged elsewhere?

Let's leave it for now. I might need to use this branch to pull out the last of the changes we haven't yet. Once everything is merged we can close.

phys-cgarnier added 8 commits March 12, 2024 10:28

init commit of screen_beam_profile.py and supporting python files (p…

79ac28f

…rojection_fit and roi), also updated image_processing.py

same

49088e7

updated measurement.py, renamed test_screen_beam_profile, added test…

eacda2a

…_measurements.py

initial commit of screen_beam_profile measurement and test notebook, …

204e14d

…plan is to build out screen_beam_profile and make test notebook a unit test.

gave docstring to measurement.py simplified test_measurement.ipynb

4545cc3

updating doc strings, and very minor details of python files

8a6adcd

updated return structure of screen_beam_profile_measurement.py, and a…

732a972

…dded to the test and example for screen_beam_profile_measurement.py

Merge branch 'dev' of https://github.com/phys-cgarnier/lcls-tools int…

a402fd8

…o dev

phys-cgarnier requested review from MattKing06, nneveu and roussel-ryan April 11, 2024 17:23

phys-cgarnier added 4 commits April 11, 2024 10:31

removed unused dependencies in an attemp to resolve some flake8 errors

a9ed3d6

more issues with flake8

26fc338

more flake8

4b0523b

black was unresolving some flake8 errors.... fixed that

b14877e

phys-cgarnier marked this pull request as draft April 12, 2024 16:39

phys-cgarnier added 2 commits April 16, 2024 09:56

changed return type type hinting for projection_fit.fit_projection si…

f2f0cb3

…nce there is a conflict with python 3.8

changed return type type hinting for projection_fit.fit_projection si…

50d6263

…nce there is a conflict with python 3.8

nneveu reviewed Apr 16, 2024

View reviewed changes

cleaning up code for PR

c4b347a

phys-cgarnier changed the title ~~Dev~~ Creating a new fitting toolkit and examples of how to use it with Screen Profiles. Apr 25, 2024

phys-cgarnier added 4 commits April 25, 2024 14:44

added docstrings in screenbeamprofile measurement started tests for r…

f955c01

…oi and image processor

updating unittests for test_image_processing and test_roi, still not …

44223a3

…quite done

updated files to have TODO comments, also completed test_image_proces…

363ed99

…sing and test_roi

updating code to no longer use distribution_data but instead use prof…

713c324

…ile_data, working through request to change self.init_values in methodBase from a list a dictionary

phys-cgarnier commented Apr 30, 2024

View reviewed changes

phys-cgarnier and others added 22 commits April 30, 2024 15:27

implemented init_values dictionary correctly.

a576c0a

code is in broken state made a branch to save changes but revert back…

89dba3d

… to working code on main

cleaning example notebook

f343813

committing working version (plots included) resolved bug, next commit…

9ee6c08

… will be used in merge

setup fitting_tool.plot_fit()

36a46bb

merged feature requests discussed in PR notes

76af0d6

moved example_measurement to examples folder

9ad190e

changed model._forward to use scipy.stats.norm, fit looks okay, shoul…

92f4011

…d maybe make norm a private variable of gaussModel.py and stop using _forward as a staticmethod

updated test_image_processing to include clipping, removed visualizat…

631541d

…iob pydantic fields.

ran black

e5cb4bf

removed whitespace and unused dependencies

69349b8

reorg fit methods, changed base model init to require data when class…

5be4f7c

… is called

fixing formatting

84abf86

one more rename of fitting files before PR

8b8b645

gaussian fit updates + test

611eda1

made changes to GaussianModel find init values.

29eef37

skipped tests in test_methods.py will fix

0523fe2

fixed flake8 syntax errors

382e502

fixed flake8 syntax errors

4ad8f45

fixed flake8 syntax errors

964bcd8

committing changes made during previous meeting

f686f49

added kwargs to projection.plot_fit so that user can compare projecti…

8c85a56

…on to fit without displaying normalized values, changed fit_model method in scipy.optimize.minimize from default (BFGS) to Powell

This was referenced Jun 12, 2024

Image Processing Class from PR 143 #171

Merged

Fitting Base Class and Gaussian Model from PR 143 #172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating a new fitting toolkit and examples of how to use it with Screen Profiles. #143

Creating a new fitting toolkit and examples of how to use it with Screen Profiles. #143

phys-cgarnier commented Apr 11, 2024

nneveu commented Apr 16, 2024 •

edited

Loading

nneveu left a comment

nneveu Apr 16, 2024

phys-cgarnier Apr 18, 2024

nneveu Apr 19, 2024

nneveu Apr 16, 2024

nneveu Apr 16, 2024

nneveu Apr 16, 2024

phys-cgarnier Apr 18, 2024

nneveu Apr 16, 2024

phys-cgarnier Apr 18, 2024 •

edited

Loading

roussel-ryan Apr 18, 2024

nneveu Apr 16, 2024

nneveu Apr 16, 2024

phys-cgarnier Apr 19, 2024

MattKing06 Apr 29, 2024

nneveu Apr 16, 2024

nneveu Apr 16, 2024

nneveu Apr 16, 2024

phys-cgarnier Apr 30, 2024 •

edited

Loading

roussel-ryan Apr 30, 2024

phys-cgarnier Apr 30, 2024

phys-cgarnier Apr 30, 2024

roussel-ryan Apr 30, 2024

phys-cgarnier Apr 30, 2024

roussel-ryan Apr 30, 2024

phys-cgarnier Apr 30, 2024 •

edited

Loading

phys-cgarnier Apr 30, 2024

roussel-ryan Apr 30, 2024

nneveu commented Aug 2, 2024

eloiseyang commented Aug 2, 2024

Creating a new fitting toolkit and examples of how to use it with Screen Profiles. #143

Are you sure you want to change the base?

Creating a new fitting toolkit and examples of how to use it with Screen Profiles. #143

Conversation

phys-cgarnier commented Apr 11, 2024

nneveu commented Apr 16, 2024 • edited Loading

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

phys-cgarnier Apr 18, 2024 • edited Loading

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

phys-cgarnier Apr 30, 2024 • edited Loading

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

phys-cgarnier Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nneveu commented Aug 2, 2024

eloiseyang commented Aug 2, 2024

nneveu commented Apr 16, 2024 •

edited

Loading

phys-cgarnier Apr 18, 2024 •

edited

Loading

phys-cgarnier Apr 30, 2024 •

edited

Loading

phys-cgarnier Apr 30, 2024 •

edited

Loading