Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improve maskCover #349

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

improve maskCover #349

wants to merge 1 commit into from

Conversation

fitoprincipe
Copy link
Member

Originally, this function was designed to compute the percentage of masked pixels inside a geometry. This allows the user to compute a similar value to "CLOUD_COVER" property in Landsat scenes or "CLOUD_COVERAGE_ASSESSMENT" in Sentinel 2 scenes, but in a specific region instead of the whole scene. When creating composites we could discard "good" pixels if we filter the collection using the cloud cover property of the scene, but using this method we can discard pixels of regions with high percentage of masked pixels.

The method has been divided in two methods:

  • maskCoverRegion: computes the value using a ee.Geometry
  • maskCoverRegions: computes the value in each ee.Feature of a ee.FeatureCollection

This method can also be helpful in the creation of composites using unbounded images, like MODIS.

The param scale is to avoid using too much resources to compute this value when a rough computation is enough.

… a geometry. maskCoverRegions does the same but inside a FeatureCollection.
region: ee.Geometry,
scale: Optional[int] = None,
band: Optional[str] = None,
proxy_value: int = -999,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cannot we use a default value instead ? like int max ? As a user that's definietely a parameter that i would not like to touch.

def maskCoverRegion(
self,
region: ee.Geometry,
scale: Optional[int] = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reducer related parameters should be regrouped under a "kwargs" parameter with a direct reference to the reducer documentation so we don't need to support them all, the user can decide. Also the reason why I used "bestEffort" in my initial implementation was to avoid to force the user to take descision for scales before the "final" reduction. I usually try to avoid setting it in earlier computation and this function will force me to.

mask, geometry = self._obj.select(0).mask(), self._obj.geometry()
cover = mask.reduceRegion(ee.Reducer.frequencyHistogram(), geometry, bestEffort=True)

band = band or 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could drop this line directly into the select, it's never used elsewhere and release 1 extra line

# we want to display this resutl as a 1 digit float
ratio = ratio.multiply(1000).toInt().divide(10)
def compute_percentage(feat):
"""function to map over the resulting table and compute the percentage from the reducer's output."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for docstring in nested function as it will never be reached by the documentation. On the other hand you could use hints to make mypy happier.

image = self.image.geetools.maskCover()
assert isclose(image.get("mask_cover").getInfo(), 99.2)

def test_deprecated_mask_cover(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's too soon to remove this one I deprecated it less than 6 month ago

@@ -1315,11 +1317,27 @@ def distance(self, other: ee.image) -> ee.Image:

return ee.Image(distance)

def maskCover(self) -> ee.Image:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this function very recently so I remain convinced it's very useful to have it available. It's specifically fun to use in a simple map in an ImageCollection.
The only constraints is to have bounded images but that's exactly the error raised by GEE console so I'm happy with it.
As a rule of thumb I think the GEE errors are now well handling unbounded images and we should not spend too much time trying to work around it. When you start doing geometric stuff one need to clip and that's all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants