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

Changing Dataset CRS #758

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

Conversation

gkahiu
Copy link
Collaborator

@gkahiu gkahiu commented Jan 4, 2023

Hi @azvoleff,

Please see the work related to #717.

For remote tasks, there is a crs key added in params that contains the CRS in WKT format using the legacy WKT1_GDAL flag. See more options here which are dependent on the version of the Proj library.

For locally run tasks, the system now validates whether the input layers are in the same projection as that defined in settings. Also, the AOI object from areaofinterest.prepare_area_of_interest now uses the CRS from settings. This (AOI) object is cascaded to the respective algorithms.

On supporting importation of datasets, this is not fully implemented as it is dependent on PR #757 but have attempted to refactor some of the sections that explicitly used WGS84. One question is to how to handle the respective input and output resolution computations since there will be no CRS conversion to WGS84 taking place as was the case before.

@gkahiu gkahiu added the Kartoza label Jan 4, 2023
@gkahiu gkahiu added this to the Kartoza Support to Dec 2022 milestone Jan 4, 2023
@gkahiu gkahiu requested a review from azvoleff January 4, 2023 10:17
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 is looking good overall @gkahiu, I think this is on the right track. One additional comment beyond those inline in the review: changing the CRS in settings breaks the code used to filter layers by their extent for the dropdowns in, for example, the "Indicator for SDG 15.3.1" tool. To address this issue the functions called within get_usable_datasets in data_io.py will need to be modified. Right now if you run a "Sub-indicators for SDG 15.3.1" job with a different CRS than WGS84, and then try to run "Indicator for SDG 15.3.1" to integrate them, the dropdowns are all empty, even though data should be available and listed in that tool.

@@ -175,14 +176,19 @@ def __init__(
self.out_res = out_res
self.out_data_type = out_data_type

# TODO: Confirm if this should be optional and if we should have
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we should default to WGS84. Using WGS84 would be consistent with past behavior of the plugin so reasonable to use it as a default.

self.iface.showOptionsDialog(currentPage=OPTIONS_TITLE)
self.iface.showOptionsDialog(self.iface.mainWindow(), currentPage=OPTIONS_TITLE)

def _validate_crs_multi_layer(self, layer_defn: list) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worthwhile to offer users an option to automatically reproject any layers that are in a CRS other than that chosen in their user settings. Otherwise there is no easy way for a user to user layers they may have produced in a different CRS (within Trends.Earth that is), without re-running the analysis. Of course there might be boundary effects, due to the reprojection, there might not be transformations among the CRS if they were user defined, etc., but worth offering it as an option if possible.

resample_mode = self.get_resample_mode(temp_vrt)
out_res = None # self.get_out_res_wgs84()
resample_mode = (
gdal.GRA_NearestNeighbour
Copy link
Contributor

@azvoleff azvoleff Jan 9, 2023

Choose a reason for hiding this comment

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

This changes the logic - in past versions of T.E. get_resample_mode was used to use either gdal.GRA_Mode for resampling categorical data or gdal.GRA_Average for continuous data when aggregating pixels to go from higher to lower resolution. Better to retain that logic here rather than always use gdal.GRA_NearestNeighbour

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