43 sofast as a service

[bbean23]

This is part 1 of 2, which extracts pieces of the SofastGUI into another class. The goal is to have a common interface to Sofast, which can be wrapped with other GUIs, scripts, or web interfaces.

closes #43
to be continued in #60

[bbean23]

In order to more easily view the pieces pulled out of SofastGUI into SofastService, I've also created PR #62

[braden6521]

This is a cool decorator, didn't know that existed!

[bbean23]

Right?! Pretty nifty. Also extremely susceptible to bugs if not used with care since by definition it produces hidden side effects.

[braden6521]

I believe the input "on_calibrating" should be "on_captured," (or the other way around). It doesn't look like SofastService doesn't have an "on_calibrating" input.

Suggested change

	on_calibrating=on_calibrating,
	on_captured=on_calibrating,

[bbean23]

I don't know how you even saw this. Nice catch! Thanks!

[braden6521]

This is 100% my error that got propagated (I don't use the "Load Gray Levels" button very often apparently). But the ImageCalibration class actually saves/loads to HDF files as "ImageCalibration/calibration_type".
See:

OpenCSP/opencsp/app/sofast/lib/ImageCalibrationAbstract.py

Line 165 in fee821d

datasets = ['ImageCalibration/calibration_type']

[bbean23]

Thanks for pointing this out. Fixed!

[braden6521]

@bbean23, these improvements look great! There are just a couple bugs I found; if you could address those when you get a chance that would be great.

Not necessary for this PR, but this reminds me of something that I've been meaning to bring up with everyone. SofastGUI, since it is a GUI, isn't unit-tested. Is there a way to do this? If not, should it live in the opencsp/... folder or somewhere else? (putting it in contrib/ doesn't feel right, so IDK). Let me know what you think, but no action necessary for this PR.

[braden6521]

@bbean23, I had some thoughts about SofastService.py. This is a bit late into your development, so sorry I didn't think of this earlier. Also feel free to disregard, this is just something to think about.

1. Do we want to consider naming SofastService.py to SofastFringeService.py because it is specific to SofastFringe (as opposed to SofastFixed?)
2. Is it worth considering merging SofastService.py and SystemSofastFringe.py into one class? I say this because it feels like there is a lot in common between the two classes. You are more knowledgeable about SofastService and you future plans for it, however, so it is totally your call. Another option which might be a better idea (also totally up to you but the more I think about it, the more I think this should happen) is to merge parts of SofastService into SystemSofastFringe. For example, SofastService.run_gray_levels_cal() kinda seems like it belongs in SystemSofastFringe because functions like capture_fringe_images() and run_camera_exposure_calibration() already live in SystemSofastFringe. (Which would totally be my fault because I originally put run_gray_levels_cal() in SofastGUI for some reason).

[bbean23]

@bbean23, I had some thoughts about SofastService.py. This is a bit late into your development, so sorry I didn't think of this earlier. Also feel free to disregard, this is just something to think about.

1. Do we want to consider naming SofastService.py to SofastFringeService.py because it is specific to SofastFringe (as opposed to SofastFixed?)

2. Is it worth considering merging SofastService.py and SystemSofastFringe.py into one class? I say this because it feels like there is a lot in common between the two classes. You are more knowledgeable about SofastService and you future plans for it, however, so it is totally your call. Another option which might be a better idea (also totally up to you but the more I think about it, the more I think this should happen) is to merge parts of SofastService into SystemSofastFringe. For example, `SofastService.run_gray_levels_cal()` kinda seems like it belongs in SystemSofastFringe because functions like `capture_fringe_images()` and `run_camera_exposure_calibration()` already live in SystemSofastFringe. (Which would totally be my fault because I originally put `run_gray_levels_cal()` in SofastGUI for some reason).

[4:05 PM] Ben
So starting with 1:

I was also considering this. Ideally SofastService will apply to both SofastFringe and SofastFixed so that it can be a singular interface to the various Sofast algorithms, but maybe that is the wrong way to think about it.

What do you think?

[4:08 PM] Braden

Yeah, that would be great if it applied to both actually. If that's the vision for it, then I'd agree it should remain SofastService.py.

[4:20 PM] Ben
Cool. And you know, maybe later we'll decide differently and have to refactor haha

Now for 2:

Would it be better to put those methods (run_gray_levels_cal) in SofastSystemFringe now, or when SofastService is extended to also work with SofastSystemFixed?

I'd say that if there is a method that is specific to SofastSystemFringe or SofastSystemFixed, then that method should go in that class. I haven't worked with SofastSystemFixed yet, but I'm assuming it doesn't have a gray levels calibration?

[4:28 PM] Braden
Hmm, I'd say whatever is easiest for you right now. The more I'm thinking about it, the more I think that run_gray_levels_cal() should have been in SystemSofastFringe all along. And yes, it's not needed for SofastFixed, only SofastFringe, so I think it's save to say it doesn't belong in SofastService. If it doesn't mess up your momentum too much, I'd vote for moving it now.

[4:29 PM] Ben
Sounds good. I'll go ahead and integrate that change

[bbean23]

@braden6521 I made some changes while I was writing unit tests. Will you please take a look at them? They're all in the commit 7edc3e1. Thanks!

[braden6521]

@bbean23, this all looks good; approved!
I even gave it a test in the optics lab today and everything seems to be working fine.

[bbean23]

Thanks for the review, Braden! I just rebased on top of develop and will merge once the checks are completed.