fixed s2MergeByTime bug, backwards compatible #11
Conversation
added functionality (backwards compatible) to filter duplicates using s2merge = 'dd' in the complete compute_inverse_depth function or s2TimeMerger = 'dd' in the getImages subfunction. Note that dd stands for the capping on days in the mosaicbyTime function
| 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 |
There was a problem hiding this comment.
We can specify also specify an allowed time window, but this works and is fast!
| @@ -30,6 +30,7 @@ def compute_inverse_depth( | |||
| pansharpen: bool = False, | |||
| skip_neighborhood_search: bool = False, | |||
| bounds_buffer: int = 10000, | |||
| s2merge: Optional[str] = None, | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
added functionality (backwards compatible) to filter duplicates using s2merge = 'dd' in the complete compute_inverse_depth function or s2TimeMerger = 'dd' in the getImages subfunction. Note that dd stands for the capping on days in the mosaicbyTime function
@Jaapel @gena, please provide feedback on whether the coding (formatting and quality) is in line with the repository and whether you agree with the changes. I checked the changes locally before opening this pull request. There were no crashes and everything worked fine on a test case in Taiwan. Original code did not changed as it was added using **args (backwards compatible).