Error thrown on first processing pair attempted during real-time processing

[tpering]

As below, note this is on poor (quite dark) imagery.

[twVolc]

This seems to be quite a common one that is crashing RTP - the GUI stops updating after this error. Seems to be something to do with dark image subtraction. It looks like self.img is NoneType? Or perhaps it is dark that is NoneType and the software is failing to find a dark image for the respective plume image. I have checked early images in this sequence though and the shutter speeds are the same as here, which would suggest that the dark image should be found - it must have been found for previous images in the sequence.

[ubdbra001]

Just stumbled across this one, it looks like that the image that it's trying to process isn't being loaded properly img.img is None, and so when it tries to run img.subtract_dark_image(0) it fails twice:

1. corr = self.img - dark - Fails because self.img is None and you can't use '-' on None
2. corr = self.img - dark.img - Fails because dark = 0 and doesn't have the property img

Need to find out why the image isn't loading properly

[twVolc]

A few thoughts:

Does this always happen with the same image - i.e. is the image file itself corrupt, or if you try loading this in separately or in post processing does it load. If corrupt, I guess we need to just catch this error and then move to the next processing pair, skipping these images.

If it's an issue with the loading function itself, would a loop trying to load the image a second time if it fails the first time work?

This is one example of where catching this error at some point (either immediately on image load in, or specifically where this dark substraction error is thrown) and then moving on to the next image pair might be a reasonable solution to ensure that something small like this doesn't crash the whole RTP. I guess this is dependent on if you manage to find a clear solution to the problem that means it won't ever happen again.

[ubdbra001]

I think the image itself is fine. I reran the same batch of images without any problem. I've only had it happen once despite processing a couple of days worth of data so I'm not sure how easy it is to catch. I'll give it a bit more time, and if I can't get it then either skipping the image or trying to reload sound like a good plan.

[twVolc]

Ok great. It's definitely an odd one, but happening frequently enough that it will be an important issue to handle. Skipping will be fine in terms of it not being critical we process absolutely every image pair.

One thing that will just need to be double checked in this case is that img_A and img_B (and then later img_A_prev and img_B_prev) have been correctly reverted back to the most recent successfully loaded pair of images - i.e. we don't want to get out of sync if img_B fails in this way but img_A doesn't and has been loaded for the new time-step - I'm sure this would get corrected when the next image pair come in, but for plume speed purposes (i.e. that uses img_A_prev -> img_A and img_B_prev -> img_B) we want to be consistent in using the same image times for both the A and B images. In other words, processing should proceed as if the erroneous image pair just didn't ever exist.

With the above in mind, a suggestion for how we deal with it (written assuming the error is in img_A):

• In PyplisWorker.load_img() we first need to store the current img_A_prev in a temporary variable that we can revert back to: img_A_prev_prev. As the code already does, img_A will be set to img_A_prev.
• Try to load the image, realise it is NoneType and catch the error
• Revert back to old data - img_A_prev becomes img_A and img_A_prev_prev becomes img_A_prev
• Throw an exception which PyplisWorker.process_pair() raises again. This is caught by PyplisWorker._processing() which calls continue to go back to the start of the processing while-loop without further execution.

The only additional part that would need adding is if this error happens in img_B, which is always loaded after img_A. At some point in the exception catching we need to make sure that img_A_prev becomes img_A and the temporary img_A_prev_prev becomes img_A_prev. In this case img_A_prev_prev needs to be saved beyond the scope of PyplisWorker.load_img(), so I'm guessing this variable will actually need to be made an attribute of PyplisWorker rather than just a local variable.

You might see a more logical solution to this, but this is just my immediate thoughts for it. An additional attempt to load the image could always be thrown in there, before reverting everything. Does this make sense?

[ubdbra001]

That does make sense, but it may be worth just splitting out the actual loading into separate function that can be run for A & B before doing any processing. If a path is specified for them and the resulting Img is None then, as you suggest, throw an error that is caught by _processing(), especially as rolling back A if B is bad will be a bit of a faff.

I'll have to have a look and see what else this may affect, and if you think this is a terrible idea do shout and I'll have a think about how to implement your suggestion.

[twVolc]

No you're right, that definitely makes sense and would make things much easier. So basically only actually overwriting img_A and img_B if two local variables that load the two new images are not None? This seems very logical. Exactly how to implement it might require a little bit of thought, as it would probably mean a reasonably large change to load_img which I think is used outside of process pair as well, so might need to think about the wider implications for that function in that case?

[ubdbra001]

My thinking was to split out the two sections of load_img(): get_img() (which is just the loading) and prep_img() (which is everything else). then just call them both from load_img(). That way other places where it is called are unchanged. Then in process_pair() replace the load_image() calls with calls to get_img(), check to see if the results are None. If they are then throw an error and if not then do the prep_img() stuff,

[twVolc]

Yeh i think that makes sense to me!

[twVolc]

Just to throw a spanner in the works. This seems like pretty much the same error (hit today) but instead of the image being NoneType it's a numpy.array...

We've got a few nested functions here and I'm not sure at what point it goes wrong. It's hard to see how we can end up with a numpy.array when the first assignment of img in load_img() is the line img = pyplis.image.Img(img_path, self.load_img_func)

[twVolc]

Sorry, I missed the fact that directly above the error in the above comment, there's the NoneType error. Seems to be an issue with cv2.imread()

[ubdbra001]

Yeah, it looks like cv2.imread() can't open one of the files, and so the img property of the Pyplis image class remains None (which is what it is set as on creation). I wonder if it's another example of the software trying to open the file before it has been fully transferred.

[alfabimsakti]

I'm not sure how but we still seem to be hitting the same error. This is from the 169-171-combined branch

[alfabimsakti]

Looking at the second half of the exception, it seems the pyplis image might somehow revert to a numpy.array if there's an error in loading? I think as well as checking whether img.img is not None, after get_img() we might need to check that the output img is a pyplis.image.img object and not a numpy.array

[alfabimsakti]

Adding this to process_pair() after get_img() to check things.

        if not isinstance(img_A, pyplis.image.Img) or not isinstance(img_B, pyplis.image.Img):
            raise FileNotFoundError('Issue loading image pair, skipping to next')

[alfabimsakti]

Error still persisting somehow... The first image attached here is a little further up the trace, but clearly relates to this error as there is no Processing pair: printed statement between these two screenshots (i.e. the error relates to processing pair 095445). I can't work out how this is getting through our checks though...

[alfabimsakti]

This seems to be happening very frequently at the moment - too frequent to get a calibration, usually within 10 minutes or so of the start of processing.

[twVolc]

Just note that I've added the probable final fix to this as a comment in #195 but this still needs to be implemented as I couldn't update that branch

[ubdbra001]

I think the reason the initial attempt didn't work is because I thought img.img would be None when the image fails to load, but it's a numpy array with None in it, so the line img.img is not None will return True even if the image has failed to load. I have a branch with an updated fix where I move the error checking to the img_load function, so it throws an exception if cv2.imread() returns a None value.

This should fix it (and catch any other times image loading through cv2.imread() fails)

[twVolc]

Your fix looks like it's more optimal and I'd assume it will work, I think we just need to test it over a reasonable length of time so that we don't inadvertently break #195, which definitely does seem to fix this issue. I'll add fixed in dev label, but keep it open in case you want reminding about your fix. Feel free to close this if you're happy with the current fix though.

[ubdbra001]

Completely agree with testing my fix before applying it. I'm happy to leave yours there for now but I think long term it would be better to apply mine; I'm a little perplexed that cv2 doesn't throw an exception when it fails to load an image correctly, and at least this way any issues will be explicitly handled (and I can include a test to cover it).

Out of interest but slightly off topic, is there a specific reason that the load_picam_png() is in its own file and not (e.g.) in the io_py.py file?

[twVolc]

Out of interest but slightly off topic, is there a specific reason that the load_picam_png() is in its own file and not (e.g.) in the io_py.py file?

Good question. I have no idea. I've had issues with circular imports before, so that's why some bits of code end up in certain places that might seem odd. But it doesn't look like that would be the case here. I'm guessing I was just following some pyplis example as I was slowly getting to grips with using that package, so followed their convention. Totally happy for this to be shifted to io_py.py to tidy things up if you think that would be better. I'd probably put it at the top if doing that, as it's a reasonably important function to understand/know where it is.

[ubdbra001]

Okay, I'll move it and put it at the top of io_py.py as you suggest.

I'm guessing I was just following some pyplis example as I was slowly getting to grips with using that package, so followed their convention.

I thought that may be the case, but have found it better to double check before moving stuff.