Dev upload vector extent

[vermeulendivan]

When using the Upload option when provided an extent for the search filter, only the first features in the vector layer considered:

The upload function will now consider all of the features and make use of a bounding box to create the extent. This is the same manner as how the selection option works when creating an extent.

[john-dupuy]

@vermeulendivan thanks for the fix. In a future PR could you add a test case for this?

[john-dupuy]

PR lgtm, pre-merge I'd like to get the OK from @mattballard-planet or @astrong19 regarding the behavior of the bounding box.

For multi-feature polygons, do we want the search to be executed against a bounding box or do we want it to be executed against only the polygons?

[astrong19]

@vermeulendivan wondering if we can change this approach to mimic how it works in Explorer and our ArcGIS Pro Add-In? In those apps, if a user selects a multi-polygon we do not draw a bounding box for them and instead execute the search through a mutli-polygon search. you can explore what Explorer is doing by examining the API {:} button in Explorer that shows the cURL of what we send to Data API.

[john-dupuy]

After discussing with Asa, we would like the search to be executed within the multi-polygon rather than within the bounding box.

Here is a sample curl that explorer sends to the data api to do that:

curl -H 'Content-Type: application/json' -X POST -d '{"filter":{"type":"AndFilter","config":[{"type":"GeometryFilter","field_name":"geometry","config":{"type":"MultiPolygon","coordinates":[[[[9.251075,14.931295],[9.251075,14.333438],[10.056218,14.333438],[10.056218,14.931295],[9.251075,14.931295]]],[[[10.951862,16.117466],[10.906083,16.115301],[10.860748,16.108826],[10.816296,16.098103],[10.773158,16.083238],[10.731752,16.064373],[10.692478,16.041693],[10.655716,16.015416],[10.62182,15.985796],[10.591118,15.953121],[10.563905,15.917707],[10.540442,15.879896],[10.520953,15.840053],[10.505624,15.798563],[10.494601,15.755827],[10.487987,15.712256],[10.485843,15.668271],[10.488186,15.624296],[10.494993,15.580754],[10.506193,15.538063],[10.521677,15.496635],[10.541293,15.456866],[10.564851,15.41914],[10.592122,15.383818],[10.622844,15.35124],[10.65672,15.321716],[10.693423,15.295532],[10.732603,15.272936],[10.773882,15.254146],[10.816865,15.239342],[10.861139,15.228665],[10.906282,15.222218],[10.951862,15.220062],[10.997441,15.222218],[11.042584,15.228665],[11.086859,15.239342],[11.129842,15.254146],[11.17112,15.272936],[11.2103,15.295532],[11.247004,15.321716],[11.280879,15.35124],[11.311601,15.383818],[11.338873,15.41914],[11.362431,15.456866],[11.382047,15.496635],[11.397531,15.538063],[11.408731,15.580754],[11.415537,15.624296],[11.417881,15.668271],[11.415737,15.712256],[11.409123,15.755827],[11.398099,15.798563],[11.382771,15.840053],[11.363282,15.879896],[11.339818,15.917707],[11.312605,15.953121],[11.281903,15.985796],[11.248008,16.015416],[11.211246,16.041693],[11.171972,16.064373],[11.130565,16.083238],[11.087428,16.098103],[11.042976,16.108826],[10.997641,16.115301],[10.951862,16.117466]]]]}},{"type":"OrFilter","config":[{"type":"AndFilter","config":[{"type":"AndFilter","config":[{"type":"StringInFilter","field_name":"item_type","config":["PSScene"]},{"type":"AndFilter","config":[{"type":"AssetFilter","config":["basic_analytic_4b"]}]}]}]},{"type":"AndFilter","config":[{"type":"StringInFilter","field_name":"item_type","config":["SkySatCollect"]}]}]},{"type":"PermissionFilter","config":["assets:download"]}]},"item_types":["PSScene","SkySatCollect"]}' https://api.planet.com/data/v1/quick-search

[vermeulendivan]

@astrong19 @john-dupuy No problem, this is a good suggested solution, especially for cases where the polgons are far from one another! I have an additional suggestion, based on this discussion and related to the Selection extent option, which I will discuss with Asa at today's meeting.

[john-dupuy]

@vermeulendivan a note Asa and I realized after playing around with it a bit more (and I think you already came to the same conclusions independently):

Before this PR, it looks like our current QGIS plugin is only able to handle the first feature in the geojson when a geojson is uploaded as an AOI. So if a user uploads a MultiPolygon as their first feature - the plugin is able to search in each of the polygons that make up the MultiPolygon (e.g. `. But if a user's geojson container more than a single feature - only the first feature will be searched in/considered.

Click for an example of each

E.g. Single feature with a MultiPolygon

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "type": "MultiPolygon",
        "coordinates": [
          [
            [
              [
                100,
                0
              ],
              [
                101.5,
                0.5
              ],
              [
                101,
                1
              ],
              [
                100,
                1
              ],
              [
                100,
                0
              ]
            ]
          ],
          [
            [
              [
                102,
                0
              ],
              [
                102.5,
                0.5
              ],
              [
                103,
                1
              ],
              [
                102,
                1
              ],
              [
                102,
                0
              ]
            ]
          ]
        ]
      }
    }
  ]
}

E.g. two features:

{
  "type": "FeatureCollection",
  "features": [
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "coordinates": [
          [
            [
              8.639911987874626,
              22.402526587332602
            ],
            [
              8.639911987874626,
              21.206756195255338
            ],
            [
              9.849379155231702,
              21.206756195255338
            ],
            [
              9.849379155231702,
              22.402526587332602
            ],
            [
              8.639911987874626,
              22.402526587332602
            ]
          ]
        ],
        "type": "Polygon"
      }
    },
    {
      "type": "Feature",
      "properties": {},
      "geometry": {
        "coordinates": [
          [
            [
              9.30032093835979,
              20.556594803585654
            ],
            [
              9.30032093835979,
              19.463537228937795
            ],
            [
              10.740602450633304,
              19.463537228937795
            ],
            [
              10.740602450633304,
              20.556594803585654
            ],
            [
              9.30032093835979,
              20.556594803585654
            ]
          ]
        ],
        "type": "Polygon"
      }
    }
  ]
}

[vermeulendivan]

@john-dupuy @astrong19 Thanks for all the feedback. We discussed this at yesterday's meeting with Asa, and will do as explained above. This approach makes a lot of sense to me. I will still leave the 'bounding box' options available, as it can still be useful for some users.
@john-dupuy Thanks for the reminder on the test function, apologies for not adding one.

[vermeulendivan]

@astrong19 @john-dupuy Just some feedback on this. Both the 'feature selection' extents and the 'upload vector file' options now has options for making use of the features itself (and not a bounding box).

For both cases I left the option to make use of a bounding box, as its still a very useful option.

Example with a selection on the layer:

If there are no selection, all features will be considered:

All that remains is the tester function, which I will have done next week.

[vermeulendivan]

Noted that when using the 'Multiple features (bounding box)' option, the user is presented with an error which states that "No features selected". Won't it make more sense to rather make it to consider all features if there are no selection? The newly added options already does this, noted it for this case while doing some testing for issues. This is also how GIS usually works with build-in tools.

[vermeulendivan]

@Samweli @john-dupuy Added test functions for all of the new features. This includes a test for selected polygons, bounding box for selected polygons, polygons from uploaded layer, and bounding box from uploaded layer.
@john-dupuy Samweli will likely be doing a review before any merging is done.

[vermeulendivan]

@Samweli Jeremy found a bug here which I will fix today/now. Already know the cause.

[vermeulendivan]

@Samweli @john-dupuy Added support for embedded gpkg files (gpkg vector files which contains more than one layer). Also added a bunch of additional tests, such as checking if a feature's geometry is valid, or that a layer contains features, etc.

Here is two examples performed on an embedded gpkg.
Polygon based:

Bounding box version:

[vermeulendivan]

@Samweli Those tests which are failing are the new tests I've added with the PR, but they run perfect if I test it locally.

[john-dupuy]

@vermeulendivan awesome work - I really like the option of choosing bounding box vs searching within each of the polygons. Tested the new feature out locally and it's working.

As for the failing tests - it looks like the actual vs expected coordinates are incorrect? When I try to run these locally I get an error:

self = <planet_explorer.gui.pe_filters.PlanetAOIFilter object at 0x7fa9bac26940>, layers = <QgsVectorLayer: '' (ogr)>

    def aoi_from_layer(self, layers):
        """Determine AOI from polygons. Considers all polygons.
        :param layers: List of QgsVectorLayers
        :type layers: list
        """
        multipart_polygon = None
>       for layer in layers:
E       TypeError: 'QgsVectorLayer' object is not iterable

But this is not the error that is occurring in the pipeline.

[vermeulendivan]

@john-dupuy Will make all the suggested changes to the tests, and will have a look at those tests failing - strange it worked fine on my side, but obviously something is going wacky!
@john-dupuy @astrong19 I also decided on making two additional changes:
I removed this "Single feature" option:

The reason for this is that when using the "Multiple features" option the user can select a single feature and do the search - so still having the "Single feature" option is not necessary anymore. Also renamed those button titles to the following:

Lastly, the original "Multiple features (bounding box)" had to have a selection otherwise the user were presented with an error that they need to select atleast one feature. I now changed it, that if there are no selection, that the option will consider all features and no longer present that error. The reasoning behind this is that is how all GIS software (and remote sensing software) works, by considering all features if there are no selection. This is also how I set up the new "Multiple features" polygon-based option, and this makes much more sense to me - otherwise a user have to everytime manually select all features if they want to search for all features.

Otherwise, thanks for the feedback! I will now make those changes to those tests and, hopefully today, merge the additional updates into the PR. This improvement started as a minor fix, but ended up being a major improvement to search functions!

[vermeulendivan]

@vermeulendivan awesome work - I really like the option of choosing bounding box vs searching within each of the polygons. Tested the new feature out locally and it's working.

As for the failing tests - it looks like the actual vs expected coordinates are incorrect? When I try to run these locally I get an error:

self = <planet_explorer.gui.pe_filters.PlanetAOIFilter object at 0x7fa9bac26940>, layers = <QgsVectorLayer: '' (ogr)>

    def aoi_from_layer(self, layers):
        """Determine AOI from polygons. Considers all polygons.
        :param layers: List of QgsVectorLayers
        :type layers: list
        """
        multipart_polygon = None
>       for layer in layers:
E       TypeError: 'QgsVectorLayer' object is not iterable

But this is not the error that is occurring in the pipeline.

@john-dupuy Found the cause of this issue, where a result of a change I made to the for the embedded layer support. Apologies for not noting this.
Tested it again on my side after fixing it, and works locally for me. But I noted that its still failing when performing the tests in github.

[john-dupuy]

I believe here we should use the fixture pe_qgis_iface rather than the iface you've imported in order to avoid the error in the pipeline:

>       iface.setActiveLayer(layer)
E       AttributeError: 'NoneType' object has no attribute 'setActiveLayer'

Here and in test_bb_aoi_from_multiple_polygons you can basically do

 def test_aoi_from_multiple_polygons(layer_dir, expected_coordinates, perform_selection, pe_qgis_iface):
    """Tests the filter for the AOI read from no selection and a
    selection on a layer loaded in QGIS. AOI calculated from each polygon.
    """
    iface = pe_qgis_iface

[john-dupuy]

Thanks for the updates @vermeulendivan - tests are working for me locally too. I see a different error in github now:

>       iface.setActiveLayer(layer)
E       AttributeError: 'NoneType' object has no attribute 'setActiveLayer'

It sounds like we may be using the incorrect iface here for the tests? I made a comment in-line about trying to use the fixture pe_qgis_iface. I would be ok merging this as is (after @Samweli has a look) and fixing the tests in a follow-up.

[john-dupuy]

LGTM - I am comfortable merging this in and fixing the tests in a follow-up. @Samweli can you review as well?

[Samweli]

@Samweli can you review as well?

Sure @john-dupuy