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

Data import enhancements #757

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from

Conversation

gkahiu
Copy link
Collaborator

@gkahiu gkahiu commented Dec 22, 2022

Incorporates enhancements in #712.

On comparing extents, the implementation uses the geometry of the selected region rather than the bounding box. It compares it (the geometry) with either the bounding box (converted into a geometry) of the input raster file or the outline geometry of the vector file. The tolerance is defined in settings to allow for adjustments if needed. I hope this approach meets the expected requirements.

Copy link
Contributor

@azvoleff azvoleff left a comment

Choose a reason for hiding this comment

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

This looks good - just a few comments inline. Also, on first running the plugin with this branch I got the below. I haven't seen that message again, so maybe just appears on first run - but any idea what leads to the below?

2022-12-22T13:32:35     WARNING    Traceback (most recent call last):
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 969, in done
              self.validate_input(value)
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 1002, in validate_input
              self.ok_clicked()
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 1140, in ok_clicked
              remap_ret = self.remap_raster(out_file, self.dlg_agg.nesting.get_list())
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1250, in remap_raster
              warp_ret = self.warp_raster(temp_tif)
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1298, in warp_raster
              target_bounds = self.extent_as_list()
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1090, in extent_as_list
              bbox = self.region_selector.region_info.geom.boundingBox()
             AttributeError: 'NoneType' object has no attribute 'boundingBox'

uris = None

if job.results.type == ResultType.RASTER_RESULTS:
uris = job.results.get_main_uris()
Copy link
Contributor

Choose a reason for hiding this comment

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

This should likely be using get_all_uris. get_main_uris may be returning vrts linked to other geotiffs (for example if one of the rasters is a TiledRaster

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done, now using get_all_uris.

LDMP/data_io.py Outdated
vector_file = self.lineEdit_vector_file.text()
if not vector_file:
return None

return qgis.core.QgsVectorLayer(
self.lineEdit_vector_file.text(), "vector file", "ogr"
Copy link
Contributor

Choose a reason for hiding this comment

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

this could use vector_file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Corrected thanks.


source_crs = lyr.crs()

# Combine the geometry for all the features
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but we'll have an issue if countries that span the dateline (like Fiji) try to import land cover data that they have defined in a series of tiles linked with VRTs (as this will merge the polygons for Fiji together, creating a massive polygon from -180 to 180 in WGS84). So we'll in the future need to find a way to use the same login of splitting polygons across the meridian that we do in other tools to allow automatic import of datasets that span the dateline, without creating a dataset that spans the globe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright, makes sense to split.

@gkahiu
Copy link
Collaborator Author

gkahiu commented Dec 22, 2022

This looks good - just a few comments inline. Also, on first running the plugin with this branch I got the below. I haven't seen that message again, so maybe just appears on first run - but any idea what leads to the below?

2022-12-22T13:32:35     WARNING    Traceback (most recent call last):
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 969, in done
              self.validate_input(value)
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 1002, in validate_input
              self.ok_clicked()
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\lc_setup.py", line 1140, in ok_clicked
              remap_ret = self.remap_raster(out_file, self.dlg_agg.nesting.get_list())
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1250, in remap_raster
              warp_ret = self.warp_raster(temp_tif)
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1298, in warp_raster
              target_bounds = self.extent_as_list()
              File "C:\Users/azvoleff/AppData/Roaming/QGIS/QGIS3\profiles\default/python/plugins\LDMP\data_io.py", line 1090, in extent_as_list
              bbox = self.region_selector.region_info.geom.boundingBox()
             AttributeError: 'NoneType' object has no attribute 'boundingBox'

Am not really sure but am suspecting its something to do with the datasets - ne_10m_admin_0_countries.shp, ne_10m_admin_1_states_provinces.shp and ne_10m_populated_places.shp. Are these normally available on first time use or have to be downloaded? I have added log messages https://github.com/ConservationInternational/trends.earth/pull/757/files#diff-f5dbeb7765ec8b8d9b12e6b28b59efad8a6b99b2adcc31566eb875508396ce21R149 to help provide some more information. It could be either the datasets are not found, layers are invalid when loaded or the search for matching features did not yield any result.

@gkahiu
Copy link
Collaborator Author

gkahiu commented Dec 22, 2022

I have updated the PR to address some of the comments.

@azvoleff
Copy link
Contributor

Thanks @gkahiu. One more issue I've noted on this one: when rasters get expanded to cover the area of interest a user has selected, the no data areas that are added are set to zero. They should be set to -32768 to indicate that they are no data. I would have thought the dstNodata argument to gdal.Warp in RasterImportWorker.work would do this, but apparently not. Any idea what the correct argument is to gdal.Warp to fix this?

@gkahiu
Copy link
Collaborator Author

gkahiu commented Dec 28, 2022

Thanks @gkahiu. One more issue I've noted on this one: when rasters get expanded to cover the area of interest a user has selected, the no data areas that are added are set to zero. They should be set to -32768 to indicate that they are no data. I would have thought the dstNodata argument to gdal.Warp in RasterImportWorker.work would do this, but apparently not. Any idea what the correct argument is to gdal.Warp to fix this?

Thanks for flagging this - the argument in gdal.warp is fine. The source of the 'zero-based' values is gdal.buildVRT which defaults to zero if no value is specified for no data. I have explicitly included this parameter VRTNodata so that any subsequent operation, including gdal.warp, on the image recognizes -32768 as nodata and cascades it forward using srcNodata and dstNodata parameters respectively. I hope this helps to address the issue, kindly confirm and let me know.

@gkahiu gkahiu mentioned this pull request Jan 4, 2023
@azvoleff
Copy link
Contributor

azvoleff commented Jan 9, 2023

Thanks @gkahiu. That fixes the nodata issue. I did notice another issue though - I was playing with importing a dataset when choosing a city as the region, with a buffer applied to that city. The extent_as_list function (which ultimately relies on the output of get_admin_bbox in visualization.py, doesn't appear to properly handle cities, or, for that matter, buffered regions. If I use a city as the region, I get an uncropped input file back, even if the city+buffer covers only a small portion of the input file. Similarly if I apply a buffer to the region I have selected (using the buffer option in the settings tool), it doesn't affect the extent of the results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Kartoza Contract 3
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

2 participants