twVolc / PyCamPermanent

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

Load cal #66

Closed twVolc closed 1 year ago

twVolc commented 1 year ago

This should make it possible to load in the calibration statistics so that calibration of images is more straightforward for subsequent processing runs. It does mean that the camera images must be processed in exactly the same way (same background modelling procedure) otherwise the calibration parameters don't hold.

Hopefully this is compatible with the new config plans, or at least will be straightforward enough to incorporate into them.

@ubdbra001 I wonder if it's possible for you to run a quick test on this - e.g. load in calibration stats you have previously generated for the Lascar data, then process images exactly as previously done and see if it produces identical emission rates using your written tests? also as discussed, if you check you're happy with the changes in this pull request that would be great.

Calibration stats can be loaded in Processing > Settings > Calibration method [Preloaded], and selecting the csv file in Processing > Settings > Calibration time series file.

It would be nice to plot these data on the calibration fit statistics plot as soon as they are loaded in, so the user can inspect how calibration changes through time.

ubdbra001 commented 1 year ago

I'll have a play about with this tomorrow and let you know how it goes

ubdbra001 commented 1 year ago

I added the conditional to the save_calibration function that fixes the issue (at least applies a sticking plaster to it), and the emission results are different, but only in places. The most obvious place is that the results from run 2 start at '2022-05-20 16:05:15' whereas all the other ones start at '2022-05-20 16:05:20'.

There are two additional rows in the 2nd run at: '2022-05-20 16:35:05' and '2022-05-20 16:35:10'

The values for the rows that do match look the same though.

twVolc commented 1 year ago

I guess save_calibration doesn't need to be run when using this setting as it would/should just regenerate the exact same statistics that have just been loaded in? So really I guess we need a statement to avoid save_calibration if cal_type is preloaded.

Also, looking at the calibration_series dataframe, it has dropped the initial 350 or so rows that don't have r2 values, is this intentional?

Yes I think I purposefully made it drop any rows that don't have stats as they're not useful. I think actually what needs to be done is edit how the stats are saved and go back through all early rows that have no calibration and assign them to the most recent calibration values - i.e. 30 minutes in or whenever a calibration first becomes available. So this is an issue associated with #15 rather than anything else. But in this update it looks for the nearest available calibration point and uses that - so the first ~30 minutes get calibrated by that first calibration point ~30 minutes in anyway, so it's not a huge issue.

ubdbra001 commented 1 year ago

I guess save_calibration doesn't need to be run when using this setting as it would/should just regenerate the exact same statistics that have just been loaded in? So really I guess we need a statement to avoid save_calibration if cal_type is preloaded.

My instinct would be to save a copy of full_calibration.csv as usual, but just make it a duplicate of the copy loaded in, so that for any proccessing run there is always a full_calibration.csv associated with it. But that is more of a personal preferance than anything.

Yes I think I purposefully made it drop any rows that don't have stats as they're not useful. I think actually what needs to be done is edit how the stats are saved and go back through all early rows that have no calibration and assign them to the most recent calibration values - i.e. 30 minutes in or whenever a calibration first becomes available. So this is an issue associated with #15 rather than anything else. But in this update it looks for the nearest available calibration point and uses that - so the first ~30 minutes get calibrated by that first calibration point ~30 minutes in anyway, so it's not a huge issue.

Gotcha, that should be easy to fix.

twVolc commented 1 year ago

Just catching back up with this - am I right in thinking that things are fine to merge? It seems to me that the issue (missing data points) doesn't relate to this branch and that actually this branch avoids that issue on the case that calibration stats are preloaded? So really we need to fix dropped data points the bug separately?

twVolc commented 1 year ago

Ah I've just seen about the conditional for the save_calibration function to work, but it looks like you've added that (perhaps to another branch?) so things will work now? Or do I need to add this update here?

ubdbra001 commented 1 year ago

Ah I've just seen about the conditional for the save_calibration function to work, but it looks like you've added that (perhaps to another branch?) so things will work now? Or do I need to add this update here?

I did it on my local version of the load_cal branch, but I'm not sure if I committed the changes. I'll have a look and if not, I'll re-add the changes and push them for you to have a look.

Regarding merging: If you're happy that the additional data points in the output aren't a problem, then I'm happy for you to merge.

ubdbra001 commented 1 year ago

Okay, there are two options to deal with the bug I found:

  1. Skip saving the calibration entirely when the calibration method is [Preloaded]
  2. Re-save the preloaded calibration used in that processing run

Do you have a preference @twVolc? I'd lean towards 2 for the sake of completeness of the results, but it could be a pain having all these files around which are effectively identical.

twVolc commented 1 year ago

I think you're right, for completeness we should go with 2. Really, we want to be able to go into any processed directory and see exactly how it has been created and interrogate (potentially replot) any products. With the shear amount of data and processed directories we could end up with we don't want to have to hunt around in other places to find parts.

ubdbra001 commented 1 year ago

Okay, that commit should fix the issue using the second option. Have a look to see if you're happy with it, and if so feel free to merge.