Image refactoring #214
Image refactoring #214
Conversation
…er to debug - creates a base ImageFit abstract class that contains image validation and allows for subclasses to modify the image fitting method - implements the ImageProjectionFit subclass that uses the ProjectionFit class to fit the x/y projections - moves all image processing into a single call inside the ImageProcessing class - replaces object properties with explicit calls to functions that do computation - makes MethodBase a pydantic class that also contains `use_priors` field to specify the use of priors instead of using a field in ProjectionFit to specify this (defaults to false)
|
Theres a similar PR draft open on this? Do you want to combine? |
|
Are you referring to #199 ? |
|
I was thinking of #128. Maybe that's too old enough we want to just close it? |
|
Yes, that PR is out of date, we should close it |
There was a problem hiding this comment.
Thanks for the PR. My main question for everyone is whether or not the bounding box penalty is useful in the general case. I think LCLS-Tools should be a general code base for physics and HLA applications that integrates easily with ML applications, but shouldn't include automation in its foundation. In my view, base class ImageFit should just package fitting with the relevant output parameters for a fit, and validation can occur in a class extending ImageFit.
If you have other thoughts, let me know. I'm open to discussion. Thanks again!
| from matplotlib import pyplot as plt, patches | ||
|
|
||
|
|
||
| def plot_image_projection_fit(fit_object, results, n_stds=3): |
There was a problem hiding this comment.
I see this is in the plotting folder, but the other files in this folder appear to be more for PyDM integration than generic plotting. I remember we decided not to include plotting functions while I was merging Chris G's contributions. @nneveu any thoughts?
|
thanks for the comments @eloiseyang, I'm working on removing the bounding box stuff now and will update the PR tomorrow morning |
There was a problem hiding this comment.
Thanks for the changes, looks good to me.
Looks like a test in test_sc_cryomodule.py is failing, though you didn't change that one in a way that would make the test fail. It looks like the test here picks a random cryomodule to verify. It probably passed once by chance, but is failing again. I'll take a look myself and let you know.
Edit:
I found the issue, there's two unexpected leading whitespaces in dictionary keys in the test tests/unit_tests/lcls_tools/superconducting/test_sc_cryomodule.py.
| self.assertTrue( | ||
| hl.is_harmonic_linearizer, msg=f"{hl} is_harmonic_linearizer is not true" | ||
| ) | ||
|
|
||
| def test_is_harmonic_linearizer_false(self): | ||
| cm = MACHINE.cryomodules[f"{randint(1, 35):02d}"] | ||
| cm = MACHINE.cryomodules[f"{randint(1, 35): 02d}"] |
There was a problem hiding this comment.
There shouldn't be a space between the colon and the number here. This changes the format string from
01 to 01. The dictionary MACHINE.cryomodules doesn't expect a leading space in its key names.
There was a problem hiding this comment.
ok I addressed it, lets see if the tests pass
| @@ -22,5 +22,5 @@ def test_pv_prefix(self): | |||
| self.assertEqual(cm.pv_prefix, "ACCL:L0B:0100:") | |||
|
|
|||
| def test_num_cavities(self): | |||
| cm = MACHINE.cryomodules[f"{randint(1, 35):02d}"] | |||
| cm = MACHINE.cryomodules[f"{randint(1, 35): 02d}"] | |||
There was a problem hiding this comment.
Sorry, there's one more down the line I didn't point out. I think the test will pass after this change.
refactor image processing and fitting class to be more general / easier to debug
use_priorsfield to specify the use of priors instead of using a field in ProjectionFit to specify this (defaults to false)