fix a bug on determining srid when upload a vector layer #437

boney-bun · 2018-06-21T19:57:50Z

boney-bun · 2018-06-21T20:18:54Z

This PR is addressing what i believe as a bug at uploading a vector layer.
The problem lies at determining the layer's srid.
It returned EPSG:None.
In the case of Africa Volcano layer, the SRS is:

GEOGCS["GCS_WGS_1984",
    DATUM["WGS_1984",
        SPHEROID["WGS_84",6378137,298.257223563]],
    PRIMEM["Greenwich",0],
    UNIT["Degree",0.017453292519943295]]

However, the geonode doesn't recognize this.
The vector layer doesn't use gdal and osr functions to determine the SRS as in raster layer.
So, we expect the SRS to be 4326.
But, the code failed to do so.
So, this PR is basically make sure that if the layer's SRS can't be determined, then it should be set to 4326.

@gubuntu ~~I understand that we should go with 3857 as the default. But, 4326 is what the upstream do.~~
[Gavin: No @boney-bun we should not go with 3857, you are confusing the data CRS with the web map client request CRS. The data can be stored in any CRS whereas web requests from Leaflet are always in 3857 -- QGIS will project on the fly]

the PR in action:

The first Africa Volcano illustrates the original behaviour when uploading a vector layer (no thumbnail and can't see the layer detail).
The second one denotes another bug on upstream (can't see the layer detail).
The last illustration indicates the PR in action (no errors).

codecov · 2018-06-21T20:44:29Z

Codecov Report

Merging #437 into 2.8.x-qgis_server will not change coverage.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##           2.8.x-qgis_server     #437   +/-   ##
==================================================
  Coverage              41.04%   41.04%           
==================================================
  Files                    403      403           
  Lines                  28706    28706           
  Branches                3648     3648           
==================================================
  Hits                   11782    11782           
  Misses                 16202    16202           
  Partials                 722      722

Impacted Files	Coverage Δ
geonode/layers/utils.py	`56.29% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ea02f2...df3faf2. Read the comment docs.

myarjunar

LGTM 👍 thanks!

myarjunar · 2018-06-22T04:44:51Z

geonode/layers/utils.py

@@ -375,7 +377,8 @@ def get_bbox(filename):
        bbox_y0 = min(ext[0][1], ext[2][1])
        bbox_x1 = max(ext[0][0], ext[2][0])
        bbox_y1 = max(ext[0][1], ext[2][1])
-        srid = srs.GetAuthorityCode(None) if srs else 'EPSG:4326'
+        # eliminate default EPSG srid as it will be added when this function returned
+        srid = srs.GetAuthorityCode(None) if srs else '4326'

    return [bbox_x0, bbox_x1, bbox_y0, bbox_y1, "EPSG:%s" % str(srid)]


even an expert geonode developer can miss this 😂

This should have been tested by unittest. If not, then we should add.

Also we need to submit PR to upstream GeoNode for issues like this.

@boney-bun cc @lucernae we should be doing all GeoNode PRs directly upstream.

Also why is this merged when we decided to not assume 4326 if no projection defined?

@gubuntu i merged this PR so others can start working on upload (including me) in the testing.
the new PR for checking the range within 4326 is in #442.
could you kindly verify it?
once satisfied, i'm going to make PR to upstream.

NyakudyaA · 2018-06-22T07:15:48Z

@boney-bun cc @myarjunar I think it is wrong to say if layer's SRS can't be determined, then it should be set to 4326.
This would result in problems later because the layer will render in a different place on the globe. I think what you should rather be doing is let the upload fail with an error message when the SRS cannot be determined.

NB: Not all projections are entered in the EPSG database but the important thing is the projection string

cape.zip
I have attached a sample dataset which is in South African local Transverse Mercator - This projection is not added in the EPSG database so you won't find the EPSG code for that but the proj string is still valid.

myarjunar · 2018-06-22T07:27:55Z

@NyakudyaA I reckon what @boney-bun did here is only to fix programming error that causes the method returned EPSG:None with the original logic is still following what upstream already did.

boney-bun · 2018-06-22T07:40:26Z

thanks @myarjunar you've got my point.

@NyakudyaA @gubuntu this is where i need an opinion from a GIS expert :)

technically speaking, the original code:
srid = layer.srs.srid if layer.srs else '4326'
is basically checking if layer.srs is empty.
in the case of Africa Volcano layer, layer.srs is not empty.
but when executing layer.srs.srid, it returned None.
geonode doesn't recognize the projection info from the vector layer.
in fact, layer.srs returned the following information:
(cmiiw, I expect the layer.srs.srid return something like: EPSG:4326 because the below info is equal to WGS84)

GEOGCS["GCS_WGS_1984",
    DATUM["WGS_1984",
        SPHEROID["WGS_84",6378137,298.257223563]],
    PRIMEM["Greenwich",0],
    UNIT["Degree",0.017453292519943295]]

so to accomodate @NyakudyaA comment, our options are:

teach geonode to recognize the above info from the vector layer, or
use gdal and osr as in raster layer.
leave it as it's from the upstream

we may need to make a PR to the upstream.

which one do you prefer @NyakudyaA @gubuntu ?

NyakudyaA · 2018-06-22T09:07:23Z

@boney-bun I think the best way you could do is do a PR against upstream with your changes.
I think you might need to consider the following:

How does the EPSG: Code gets used in the front end to render the layer.
Instead of using the EPSG: Code can you use layer.srs.proj4. This should cover all cases where the EPSG code is not defined in the projection file.
I know when using gdal to project layers to different formats you can use either the EPSG code or the proj4 string and still the results will generate the layer in the same position.

…ction

codecov-io · 2018-06-23T08:37:40Z

Codecov Report

Merging #437 into 2.8.x-qgis_server will increase coverage by 0.01%.
The diff coverage is 81.81%.

@@                  Coverage Diff                  @@
##           2.8.x-qgis_server     #437      +/-   ##
=====================================================
+ Coverage              41.04%   41.05%   +0.01%     
=====================================================
  Files                    403      403              
  Lines                  28706    28715       +9     
  Branches                3648     3648              
=====================================================
+ Hits                   11782    11789       +7     
- Misses                 16202    16204       +2     
  Partials                 722      722

Impacted Files	Coverage Δ
geonode/layers/utils.py	`56.66% <81.81%> (+0.36%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ea02f2...45e0ec4. Read the comment docs.

boney-bun · 2018-06-23T08:48:16Z

Hi @NyakudyaA

I've changed the code a bit.
It seems that gdal's DataSource is unreliable to determine the epsg code (the function reads the shapefile).
So, I change to reading a prj file and it can now detect the epsg code properly.
This PR seems to partially solve #395 too as illustrated below:

For 4326 it can also be uploaded successfully:

@NyakudyaA @gubuntu please review the code from the GIS perspective. then, i could make a pr to the upstream.

NyakudyaA

When EPSG: code is not found it should not be assumed to be 4326. Let's rather default to using the proj4 text to define the projection. See how the EPSG:Code is being used by the front end.

lucernae

LGTM. On a side note, does the previous code works with GeoNode dataset? Because it's hard to imagine that this passed their unittest.

lucernae · 2018-06-29T07:28:48Z

geonode/layers/utils.py

@@ -375,7 +377,8 @@ def get_bbox(filename):
        bbox_y0 = min(ext[0][1], ext[2][1])
        bbox_x1 = max(ext[0][0], ext[2][0])
        bbox_y1 = max(ext[0][1], ext[2][1])
-        srid = srs.GetAuthorityCode(None) if srs else 'EPSG:4326'
+        # eliminate default EPSG srid as it will be added when this function returned
+        srid = srs.GetAuthorityCode(None) if srs else '4326'

    return [bbox_x0, bbox_x1, bbox_y0, bbox_y1, "EPSG:%s" % str(srid)]


This should have been tested by unittest. If not, then we should add.

lucernae · 2018-06-29T07:29:32Z

geonode/layers/utils.py

@@ -375,7 +377,8 @@ def get_bbox(filename):
        bbox_y0 = min(ext[0][1], ext[2][1])
        bbox_x1 = max(ext[0][0], ext[2][0])
        bbox_y1 = max(ext[0][1], ext[2][1])
-        srid = srs.GetAuthorityCode(None) if srs else 'EPSG:4326'
+        # eliminate default EPSG srid as it will be added when this function returned
+        srid = srs.GetAuthorityCode(None) if srs else '4326'

    return [bbox_x0, bbox_x1, bbox_y0, bbox_y1, "EPSG:%s" % str(srid)]


Also we need to submit PR to upstream GeoNode for issues like this.

… unit test

boney-bun · 2018-06-30T17:14:11Z

thanks for the review @NyakudyaA @myarjunar @lucernae
it has passed the travis so I'm going to merge them.

fix a bug on determining srid when upload a vector layer

df3faf2

fix kartoza#289 fix kartoza#435

boney-bun requested review from gubuntu, lucernae and NyakudyaA June 21, 2018 19:57

boney-bun added the in progress label Jun 21, 2018

myarjunar approved these changes Jun 22, 2018

View reviewed changes

replace GDAL's DataSource with osr function for proper EPSG code dete…

a3cd0c0

…ction

NyakudyaA reviewed Jun 25, 2018

View reviewed changes

lucernae approved these changes Jun 29, 2018

View reviewed changes

change the error message when prj file not present so it can pass the…

45e0ec4

… unit test

boney-bun merged commit 30828ab into kartoza:2.8.x-qgis_server Jun 30, 2018

boney-bun removed the in progress label Jun 30, 2018

gubuntu mentioned this pull request Jul 19, 2018

[GeoNode] Projection issue in the layers #395

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix a bug on determining srid when upload a vector layer #437

fix a bug on determining srid when upload a vector layer #437

boney-bun commented Jun 21, 2018

boney-bun commented Jun 21, 2018 •

edited by gubuntu

Loading

codecov bot commented Jun 21, 2018 •

edited

Loading

myarjunar left a comment

myarjunar Jun 22, 2018

lucernae Jun 29, 2018

lucernae Jun 29, 2018

gubuntu Jul 4, 2018

boney-bun Jul 5, 2018

NyakudyaA commented Jun 22, 2018

myarjunar commented Jun 22, 2018

boney-bun commented Jun 22, 2018

NyakudyaA commented Jun 22, 2018

codecov-io commented Jun 23, 2018 •

edited by codecov bot

Loading

boney-bun commented Jun 23, 2018

NyakudyaA left a comment

lucernae left a comment

lucernae Jun 29, 2018

lucernae Jun 29, 2018

boney-bun commented Jun 30, 2018

fix a bug on determining srid when upload a vector layer #437

fix a bug on determining srid when upload a vector layer #437

Conversation

boney-bun commented Jun 21, 2018

boney-bun commented Jun 21, 2018 • edited by gubuntu Loading

codecov bot commented Jun 21, 2018 • edited Loading

Codecov Report

myarjunar left a comment

Choose a reason for hiding this comment

myarjunar Jun 22, 2018

Choose a reason for hiding this comment

lucernae Jun 29, 2018

Choose a reason for hiding this comment

lucernae Jun 29, 2018

Choose a reason for hiding this comment

gubuntu Jul 4, 2018

Choose a reason for hiding this comment

boney-bun Jul 5, 2018

Choose a reason for hiding this comment

NyakudyaA commented Jun 22, 2018

myarjunar commented Jun 22, 2018

boney-bun commented Jun 22, 2018

NyakudyaA commented Jun 22, 2018

codecov-io commented Jun 23, 2018 • edited by codecov bot Loading

Codecov Report

boney-bun commented Jun 23, 2018

NyakudyaA left a comment

Choose a reason for hiding this comment

lucernae left a comment

Choose a reason for hiding this comment

lucernae Jun 29, 2018

Choose a reason for hiding this comment

lucernae Jun 29, 2018

Choose a reason for hiding this comment

boney-bun commented Jun 30, 2018

boney-bun commented Jun 21, 2018 •

edited by gubuntu

Loading

codecov bot commented Jun 21, 2018 •

edited

Loading

codecov-io commented Jun 23, 2018 •

edited by codecov bot

Loading