twVolc / PyCamPermanent

Permanent PiCam (SO2) installation project software
GNU General Public License v3.0
1 stars 2 forks source link

Create a `SpecWorker` class to act as parent for `IFitWorker` and `DOASWorker` #150

Closed ubdbra001 closed 3 weeks ago

ubdbra001 commented 1 month ago

These changes move the common features of IFitWorker and DOASWorker into SpecWorker, as outlined in #136.

It should also mean that new features added to SpecWorker will be available to both child spectrum processing classes, and make code maintenance easier.

There were some methods that were similar between the two, but not exactly the same, and these were left separate. E.g.:

twVolc commented 1 month ago

Not sure if these notes are useful or if you're purposefully not wanting to include other methods in SpecWorker. But here's just a summary:

start_watching() can be exactly duplicated - following the IFitWorker current design. for the print statements, perhaps both DOASWorker and IFitWorker can have their own attribute processing_type which is a string DOAS and iFit respectively, and this attribute can be inserted into the print strings where relevant, to indicate which worker is printing that statement. Alternatively the statements can be moved back to the DOASWorker style, or they could each just have the base SpecWorker as the string, this would be fine (prints statements might be removed eventually anyway so this isn't something to get caught up on).

start_processing_threadless() again this can be duplicated, but might require a few updates - e.g. _process_loop() in DOASWorker would need to take the continuous_save input. Maybe it could just take it for now but not do anything with it - making that all work feels like it would be a lot more work. STOP_FLAG would need to be a DOASWorker attribute, so would just need to be made a SpecWorker attribute.

make_doas_results() should probably just duplicate IFitWorker, but I think either way the line doas_results.ldfs = [np.nan] * len(stds) should be changed to doas_results.ldfs = [np.nan] * len(column_densities). And looking at it I think this should probably be done anyway, as this method would raise an unwanted error if stds remains as None and we try len(stds). This is probably a bug waiting to happen...

rem_doas_results() - if the above changes are made to make_doas_results(), the IFitWorker version of this will be compatible with DOASWorker as the ldfs will just be NaNs and that's fine. So again this could be moved to SpecWorker.

find_dark_spectrum(). Go with the IFitWorker version - I'll have worked with this one more recently so there will be a reason I've made those minor changes - probably through finding bugs or undesired results with the DOASWorker version.

ubdbra001 commented 1 month ago

For this I'm going for the really quick wins, and so just leaving these methods alone for now. But this information is really useful, so I'll add it to another issue and if/when either of us return to it we can use it.

twVolc commented 1 month ago

Ok great. Should we be creating a new dev branch or is it fine to go back to the same branch even though that has been merged with master now?

ubdbra001 commented 1 month ago

I'm happy with it just being added to the same dev branch, I don't think there's any harm in reusing it.

twVolc commented 1 month ago

Cool. Are there any iFit worker tests setup at the moment to ensure the processing stays the same? If so and it has passed them I'm happy to merge this

ubdbra001 commented 1 month ago

No formal tests yet, but I did a run with the data and settings I've been using and it produces identical results to the current release. Might still be worth you giving that a go too, just to cover off any parts I may not be using with my run.