Couple of changes/enhancements #135

wouellette · 2019-03-19T13:25:30Z

Hi Drew,

Sorry for taking so long to get back to this, but in the mean time I have been working on my own head and I came up with more than just the "label_area" improvement suggested in the previous pull request, so decided to create a new one here to discuss the additional changes I came up with, and that I think would be of common interest, especially the support for create labels across country borders. See list of improvements below:

country can be provided as list() instead of single string, thus allowing the download and label generation for multiple countries in one go. This stemmed from the need to generate labels for AOIs overlapping two countries.
The initial "label_area" suggestion for the ml_type=classification from Add "label_area" attribute to classification.geojson #128 is taken up here.
Changed the check for geojson availability, because I didn't understand the initial geojson attribute of providing one's own labelled features. When would that be the case? I interpreted it as an alternative to providing the AOI instead of the bounding_box+country combination. Maybe keeping the original geojson attribute and adding an aoi attribute to cover my intended functionality could make sense.
Create labels based on multiple country .mbtile files (related to first point where countries can be provided as list).

Let's discuss on the common ground for this pull request, what could be taken up from what couldn't, and once we agree I can work on the tests.

Cheers,

William

update

If multiple countries are provided as part of list, or if AOI overlaps two countries, downloads across borders.

aoi_name is useful to give a name extension to the classification.geojson. country as list allows downloading of multiple countries at once.

Converted it to ares to roughly match a Sentinel-2 pixel size.

wouellette · 2019-03-19T13:27:43Z

label_maker/download.py

-        with open(download_file, 'wb') as w:
-            for line in r:
-                w.write(line)
+    for ctr in country:


Iterating over country list

wouellette · 2019-03-19T13:34:46Z

label_maker/label.py

-    mbtiles_file_zoomed = op.join(dest_folder, '{}-z{!s}.mbtiles'.format(country, zoom))
-
-    if not op.exists(mbtiles_file_zoomed):
-        filtered_geo = kwargs.get('geojson') or op.join(dest_folder, '{}.geojson'.format(country))


Here, I am not sure I understand why kwargs.get('geojson') is picked up here for the variable. This would assume that the geojson provided in the config.json file contains the label features on which to generate the labelled output for classification, object detection or segmentation.

However, I cannot see a use case where someone would provide their labelled features as a standalone geojson file, because then would they really need to use the label-maker?
If we keep the geojson attribute in its current defintion, I still see the value of an aoi field to provide the AOI in the form of geometries, as an alternative to the country+bbox combination.

@wouellette the geojson attribute is provided so users can add a standalone GeoJSON file. label-maker still provides value here in that it will chip up the imagery and GeoJSON labels, rasterize or create object labels, etc. We need to keep this attribute as it's used in a number of existing workflows.

wouellette · 2019-03-19T13:36:49Z

label_maker/main.py

@@ -85,12 +85,11 @@ def cli():

    # custom validation for top level keys
    # require either: country & bounding_box or geojson
-    if 'geojson' not in config.keys() and not ('country' in config.keys() and 'bounding_box' in config.keys()):
-        raise Exception('either "geojson" or "country" and "bounding_box" must be present in the configuration JSON')
+    if not ('country' in config.keys() and 'geojson' in config.keys()) and not ('country' in config.keys() and 'bounding_box' in config.keys()):


This change relates to the change of the filtered_geo variable, where I interpreted the geojson attribute to be an alternative to provide the AOI instead of country+bbox combination.

drewbo · 2019-03-19T14:45:38Z

@wouellette 👍 on multiple countries and the label_area addition, see my comments inline for the geojson use case (the implementation is a bit kludgy but works for now)

wouellette · 2019-03-19T15:45:32Z

Apology on the clumsy implementation, I was planning on reworking it, but just wanted to discuss the value of the changes first.

About the geojson attribute, it's well understood. Now, would an aoi geometry attribute (provided as geojson or other format) as an alternative to bounding_box make sense? I personally use it to guarantee that the labelled output has complete coverage over the expected geographic extent, and it's more natural to pass a file to configuration than a Bbox if you have a very specific area over which to generate labels.

wouellette · 2019-03-19T19:18:16Z

label_maker/label.py

+            label_bool2 = label_bool
+            label_area2 = label_area
+            if ctr == country[-1]:
+                json.dump(fc(features), open(op.join(dest_folder, f'classification_{zoom}.geojson'), 'w'))


added zoom to file name to identify the zoom level at which labelled output was generated.

drewbo · 2019-03-20T13:30:14Z

Ha, apologies, I meant that my implementation of the geojson key was the issue, not your new code. Thanks again for the contribution and I'll take a look sometime this week

label_maker/label.py

drewbo · 2019-04-03T21:40:42Z

@wouellette giving a more thorough look now. Can you add a test case for this also? It breaks all of the tests now because of the country list change and change to the output; I can help with updating those

wouellette · 2019-05-17T21:05:43Z

Better late than never, I updated the test cases, but the WMS endpoint is still failing because the usgs wms used for the test is not matching the fixture tile. I guess they updated the imagery in usgs to cause this mismatch?

Edit: I am working on the integration tests too FYI.

drewbo · 2019-05-17T21:25:39Z

@wouellette awesome; if you pull in master I updated the WMS test to point to a new endpoint

sync with original repo

# Conflicts: # requirements.txt

wouellette · 2019-06-05T16:02:24Z

The integration tests were also updated.

They all passed, but I had to update the label tile outputs as the former ones were reflecting the presence/absence with 1s and 0s of a class, whereas now it is the surface area which is represented (values other than 0s and 1s).

drewbo · 2019-06-24T15:08:31Z

@wouellette thanks, I will likely get a chance to check this out this week; can you remove your venv files from the PR?

Change base SentinelHub layer, close developmentseed#139

drewbo · 2019-06-25T13:57:54Z

@wouellette gave a quick look today and looks really solid. Two other requests:

Can you add the ctr/ctr_idx iteration to the object detection and segmentation cases? Right now it looks like those would fail with two countries.
Related to the above, we should have test cases for multiple countries for all ml_types. Let me know if you'd like me to create any of those.

wouellette · 2019-06-26T08:38:11Z

Regarding your two requests:

The object detection and segmentation cases were adapted, the only thing was that the syntax for img.paste() was incorrect so I adapted it so if a .png has already been created, it pastes the box or the geometry in rather than overwrite the whole .png.
I adapted the tests for an area across the border between Spain and Portugal for the standard ml_type cases (no parsing, country+bbox combo). Because the OSM QA tiles are buffered beyond the countries' borders, I had to take a larger bounding_box and take a lower zoom level to have a limited amount of outputs (24 tiles at zoom level 10). I kept the single country test at zoom level 17 for portugal for the sparse cases, as well as the geojson and aoi cases, so that both the single and double country cases were tested.

wouellette and others added 15 commits January 30, 2019 12:27

Merge pull request #1 from developmentseed/master

08dc85c

update

Enables download for multiple countries at once

b0cdbe1

If multiple countries are provided as part of list, or if AOI overlaps two countries, downloads across borders.

added aoi_name and country as list

a75fcf8

aoi_name is useful to give a name extension to the classification.geojson. country as list allows downloading of multiple countries at once.

added feat_id and label_area attributes to classification.geojson

7c78d0b

Update label.py

bc1aedc

Update label.py

4dfe770

Update label.py

d5c600e

Convert label_area unit

9f8e950

Converted it to ares to roughly match a Sentinel-2 pixel size.

Removed injection to postgis table

6492904

improved segmentation ml_type to output shapes to geojson

da1d2b7

allow download and labels generation for multiple countries

1d4be9e

removed useless loading of geojson

6b4a4be

Re-activate the creation of the labels.npz file

336b1c8

revert to country

a881781

remove format attribute

107ab62

wouellette mentioned this pull request Mar 19, 2019

Add "label_area" attribute to classification.geojson #128

Closed

wouellette commented Mar 19, 2019

View reviewed changes

wouellette added 2 commits March 19, 2019 14:28

removed residual libraries

d8efeb8

remove redundant library

5b5a601

wouellette commented Mar 19, 2019

View reviewed changes

wouellette changed the title ~~Couple of enhancements~~ Couple of changes/enhancements Mar 19, 2019

wouellette commented Mar 19, 2019

View reviewed changes

wouellette added 3 commits March 19, 2019 21:47

re-established geojson intended functionality

366f99a

added aoi config key

5fa230d

added aoi config key

d08c480

drewbo reviewed Apr 3, 2019

View reviewed changes

label_maker/label.py Outdated Show resolved Hide resolved

wouellette and others added 3 commits April 8, 2019 18:45

Cleaner implementation of the classification ml_type

5ff5573

minor fixes

c5dfb32

updated unit tests

c559fcf

updated unit tests

81d7ba6

wouellette force-pushed the master branch from 0d87e0d to 81d7ba6 Compare May 17, 2019 22:39

wouellette and others added 6 commits May 18, 2019 00:41

reverted to previous requirements

7bb1836

Update requirements.txt

3427220

Merge pull request #3 from developmentseed/master

66e3e2a

sync with original repo

Merge branch 'master' of https://github.com/wouellette/label-maker

7f1a9b6

# Conflicts: # requirements.txt

finalized integration tests

ac3bbca

cosmetic pylint changes

94c24b0

additional pylint changes

a6a70ac

wouellette and others added 2 commits June 24, 2019 19:48

removed venv directory

de0e80f

Merge pull request #4 from developmentseed/master

b3f4b96

Change base SentinelHub layer, close developmentseed#139

added integration tests with two countries as input for all ml_types

dafa90e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Couple of changes/enhancements #135

Couple of changes/enhancements #135

wouellette commented Mar 19, 2019 •

edited

Loading

wouellette Mar 19, 2019

wouellette Mar 19, 2019

drewbo Mar 19, 2019

wouellette Mar 19, 2019

drewbo commented Mar 19, 2019

wouellette commented Mar 19, 2019

wouellette Mar 19, 2019

drewbo commented Mar 20, 2019

drewbo commented Apr 3, 2019

wouellette commented May 17, 2019 •

edited

Loading

drewbo commented May 17, 2019

wouellette commented Jun 5, 2019

drewbo commented Jun 24, 2019

drewbo commented Jun 25, 2019

wouellette commented Jun 26, 2019 •

edited

Loading

Couple of changes/enhancements #135

Are you sure you want to change the base?

Couple of changes/enhancements #135

Conversation

wouellette commented Mar 19, 2019 • edited Loading

wouellette Mar 19, 2019

Choose a reason for hiding this comment

wouellette Mar 19, 2019

Choose a reason for hiding this comment

drewbo Mar 19, 2019

Choose a reason for hiding this comment

wouellette Mar 19, 2019

Choose a reason for hiding this comment

drewbo commented Mar 19, 2019

wouellette commented Mar 19, 2019

wouellette Mar 19, 2019

Choose a reason for hiding this comment

drewbo commented Mar 20, 2019

drewbo commented Apr 3, 2019

wouellette commented May 17, 2019 • edited Loading

drewbo commented May 17, 2019

wouellette commented Jun 5, 2019

drewbo commented Jun 24, 2019

drewbo commented Jun 25, 2019

wouellette commented Jun 26, 2019 • edited Loading

wouellette commented Mar 19, 2019 •

edited

Loading

wouellette commented May 17, 2019 •

edited

Loading

wouellette commented Jun 26, 2019 •

edited

Loading