search

cloudsci / cloudmetrics

Toolkit for computing 15+ metrics characterising 2D cloud patterns
16 stars 8 forks source link

Handle fully cloudy in open-sky metric #67

Closed leifdenby closed 1 year ago

leifdenby commented 1 year ago

@martinjanssens in using cloudmetric I came across where one of the metrics currently fails. That when we try to compute the mean of the open-sky metric where there are no "open" regions in the mask :D (due to a divide-by-zero error) I think the case of a fully-cloudy mask we should just return 0 for the open-sky metric, what do you think?

leifdenby commented 1 year ago

I've added a test which exposes the bug @martinjanssens :)

martinjanssens commented 1 year ago

Ooh that's ugly... Should have spotted that, but I never encountered a completely cloudy scene in the trades so far! Completely agree that it is natural to expect this to return 0 in the case of fully cloudy scenes (just like it will return 1 when the scene is entirely cloudless). I guess we could even test for this at the very start of the function and just have it immediately return 0 if it's completely cloudy, as it wouldn't even have to loop over the pixels in the scene to know that (and this metric is by far the slowest to calculate...)?

leifdenby commented 1 year ago

I guess we could even test for this at the very start of the function and just have it immediately return 0 if it's completely cloudy, as it wouldn't even have to loop over the pixels in the scene to know that (and this metric is by far the slowest to calculate...)?

Yes, I was thinking the same! I've done the in the commit I just added.

You just mentioning to me about things being slow, I just realised that we can probably get away with optimising a lot of the metric functions with numba since most routines use just standard python primitive types and numba types. I just had a go at decorating the open_sky function with @numba.jit and the tests still pass :) So, maybe we should do that? I'll open a new issue to keep that in mind. Should give quite a speed boost.

observingClouds commented 1 year ago

Could this be merged as the performance seems to be discussed over in #68 ? I just came across this as well (mainly though because of a missing field rather than no-clouds).