-
Notifications
You must be signed in to change notification settings - Fork 45
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
base: develop
Are you sure you want to change the base?
Data import enhancements #757
Conversation
for more information, see https://pre-commit.ci
…trends.earth into data_import_enhancements
There was a problem hiding this 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'
LDMP/jobs/manager.py
Outdated
uris = None | ||
|
||
if job.results.type == ResultType.RASTER_RESULTS: | ||
uris = job.results.get_main_uris() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
I have updated the PR to address some of the comments. |
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 |
Thanks for flagging this - the argument in |
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 |
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.