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.

Sign up for GitHub

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

fixed s2MergeByTime bug, backwards compatible #11

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fixed s2MergeByTime bug, backwards compatible #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
946b649
fixed s2MergeByTime bug, backwards compatible
EtienneKras Mar 4, 2022
38157ec
minor change to the mosaic by time function to be consistent
EtienneKras Mar 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions eepackages/applications/bathymetry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ def compute_inverse_depth(
pansharpen: bool = False,
skip_neighborhood_search: bool = False,
bounds_buffer: int = 10000,
s2merge: Optional[str] = None,
Copy link
Collaborator

@gena gena Mar 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure why should we expose internal knowledge about S2 to the outside options, I'd rather keep this encapsulated (hidden within the package) ... but we already have s2MergeByTime ... let's maybe discuss what we're trying to achieve here

Copy link
Collaborator Author

@EtienneKras EtienneKras Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

What I was trying to achieve is to correctly filter out S2 images as I still got multiple images at the same time. We were not sure whether this was a bug or something that you were aware of. Therefore, in discussion with Jaap, I implemented it backwards compatible for the time being. In this way, we could discuss with you if we would keep it like this or implement the change in the code in general. Lets indeed discuss if you still have questions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This functionality should be already there by default, in ee-packages > assets. If it's buggy - we need to fix it there instead of exposing it to the bathymetry package (SRP). I can imagine there are bugs, so may require some debugging.

) -> ee.Image:
images: ee.ImageCollection = self.get_images(
bounds=bounds,
Expand All @@ -38,7 +39,8 @@ def compute_inverse_depth(
filter_masked=filter_masked,
scale=scale,
missions=missions,
cloud_frequency_threshold_delta=cloud_frequency_threshold_data
cloud_frequency_threshold_delta=cloud_frequency_threshold_data,
s2merge=s2merge
)
# save loaded images in class as raw_images
self._raw_images = images
Expand Down Expand Up @@ -377,7 +379,8 @@ def get_images(
missions: List[str],
filter_masked_fraction: Optional[float] = None,
cloud_frequency_threshold_delta: float = 0.15,
filter: Optional[ee.Filter] = None
filter: Optional[ee.Filter] = None,
s2merge: Optional[str] = None,
) -> ee.ImageCollection:
date_filter: ee.Filter = ee.Filter.date(start, stop)
if filter:
Expand All @@ -391,7 +394,8 @@ def get_images(
"filterMasked": filter_masked,
"filterMaskedFraction": filter_masked_fraction,
"scale": ee.Number(scale).multiply(10), # why *10?
"resample": True
"resample": True,
"s2TimeMerger": s2merge,
}

images: ee.ImageCollection = assets.getImages(bounds, options_get_images)
Expand Down
24 changes: 19 additions & 5 deletions eepackages/assets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,12 @@ def getImages(g, options):

if options and 's2MergeByTime' in options:
s2MergeByTime = options['s2MergeByTime']


s2TimeMerger = False

if options and 's2TimeMerger' in options:
s2TimeMerger = options['s2TimeMerger']

cloudMask = False

if options and 'cloudMask' in options:
Expand Down Expand Up @@ -200,7 +205,7 @@ def clipNegativeFootprint(i):

# merge by time (remove duplicates)
if s2MergeByTime:
s2 = mosaicByTime(s2)
s2 = mosaicByTime(s2, s2TimeMerger)

def f2(i):
return (i
Expand Down Expand Up @@ -309,8 +314,17 @@ def f(i):
# *
# * This function mosaics images by time.
# */
def mosaicByTime(images):
TIME_FIELD = 'system:time_start'
def mosaicByTime(images, *args):
if 'dd' in args: # for rounding up until days
def f(i):
dateField = ee.Date(i.get('system:time_start')).format('YYYYMMdd') #hhmmss.SSS rounded to a minute
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can specify also specify an allowed time window, but this works and is fast!

return i.set({'dateField':dateField})

images = images.map(f)

TIME_FIELD = 'dateField'
else:
TIME_FIELD = 'system:time_start'

distinct = images.distinct([TIME_FIELD])

Expand All @@ -323,7 +337,7 @@ def merge_matches(i):
matchedImages = ee.ImageCollection.fromImages(i.get('matches'))
mosaic = matchedImages.sort('system:index').mosaic().set({ 'system:footprint': matchedImages.geometry() })

return mosaic.copyProperties(i).set(TIME_FIELD, i.get(TIME_FIELD))
return mosaic.copyProperties(i).set('system:time_start', i.get('system:time_start')) # hard-coded time_start instead of TIME_FIELD to be consistent

results = results.map(merge_matches)

Expand Down