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

FEAT: Add new geospatial functions to OmniSciDB backend #1836

Merged
merged 1 commit into from Jun 24, 2019

Conversation

2 participants
@xmnlab
Copy link
Collaborator

commented Jun 17, 2019

Add 2 new geo spatial functions to OmniSciDB backend:

  • set_srid
  • transform

Add a cast to geographic type

@xmnlab xmnlab changed the title FEAT: Added new geospatial functions to OmniSciDB backend FEAT: Add new geospatial functions to OmniSciDB backend Jun 17, 2019

@codecov

This comment has been minimized.

Copy link

commented Jun 19, 2019

Codecov Report

Merging #1836 into master will decrease coverage by 1.77%.
The diff coverage is 96.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1836      +/-   ##
==========================================
- Coverage   87.46%   85.69%   -1.78%     
==========================================
  Files          81       81              
  Lines       15458    15501      +43     
  Branches     1977     1985       +8     
==========================================
- Hits        13520    13283     -237     
- Misses       1571     1853     +282     
+ Partials      367      365       -2
Impacted Files Coverage Δ
ibis/expr/operations.py 95.92% <100%> (ø) ⬆️
ibis/expr/datatypes.py 95.31% <100%> (+0.17%) ⬆️
ibis/expr/api.py 92.32% <100%> (-0.05%) ⬇️
ibis/mapd/operations.py 72.08% <60%> (+0.76%) ⬆️
ibis/bigquery/client.py 41.1% <0%> (-53.39%) ⬇️
ibis/bigquery/compiler.py 59.92% <0%> (-37.5%) ⬇️
ibis/bigquery/udf/api.py 80.48% <0%> (-14.64%) ⬇️
ibis/bigquery/api.py 63.33% <0%> (-10.01%) ⬇️
ibis/impala/compiler.py 91.29% <0%> (-5.26%) ⬇️
ibis/pandas/client.py 85.54% <0%> (-3.47%) ⬇️
... and 5 more

@xmnlab xmnlab force-pushed the Quansight:add-more-geo-funcs-mapd branch 2 times, most recently from b8e88b3 to a6704ec Jun 19, 2019

@xmnlab xmnlab marked this pull request as ready for review Jun 19, 2019

@xmnlab xmnlab requested a review from cpcloud Jun 20, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

@cpcloud it seems it is done for a review .. there is just one error related to impala test_drop_view - ibis.impala.tests.test_ddl

Show resolved Hide resolved ibis/expr/datatypes.py Outdated
Show resolved Hide resolved ibis/expr/datatypes.py Outdated
Show resolved Hide resolved ibis/expr/operations.py Outdated
dtype_from = arg._dtype.geotype
dtype_target = target.geotype

if dtype_from == dtype_target:

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

Why is this an error and not a no-op?

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 20, 2019

Author Collaborator

I think I am not following you here. could you provide more details?

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

I think I'm not understanding why it's an error to cast from a geotype to the same geotype. That is what you're doing here is it not?

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 20, 2019

Author Collaborator

Oh I see .. you're right. I will fix that! thanks!

Show resolved Hide resolved ibis/tests/all/test_geospatial.py Outdated
Show resolved Hide resolved ibis/tests/all/test_geospatial.py Outdated
Show resolved Hide resolved ibis/tests/all/test_geospatial.py Outdated
Show resolved Hide resolved ibis/tests/all/test_geospatial.py Outdated
Show resolved Hide resolved ibis/tests/backends.py Outdated
@xmnlab
Copy link
Collaborator Author

left a comment

Thanks @cpcloud for the feedback.
I will apply the changes you requested.
I added some comments and question for some topics here. thanks!

Show resolved Hide resolved ibis/tests/all/test_geospatial.py Outdated
Show resolved Hide resolved ibis/tests/backends.py Outdated
Show resolved Hide resolved ibis/expr/datatypes.py Outdated
Show resolved Hide resolved ibis/expr/datatypes.py Outdated
Show resolved Hide resolved ibis/expr/operations.py Outdated
dtype_from = arg._dtype.geotype
dtype_target = target.geotype

if dtype_from == dtype_target:

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 20, 2019

Author Collaborator

I think I am not following you here. could you provide more details?

@xmnlab xmnlab force-pushed the Quansight:add-more-geo-funcs-mapd branch from a6704ec to 7ebcecf Jun 20, 2019

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

@xmnlab Looks like conda-forge is not working at the moment, lots of 503s.

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

@cpcloud it seems yesterday conda-forge was also unstable ...

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

@cpcloud I fix the code related to cast with same type (#1836 (comment)) let me know your thoughts about the approach used.

target_type.lower() in ('geometry', 'geography')
and (
arg.type().geotype == target_type
or (arg.type().geotype is None and target_type == 'geometry')

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

Can you explain the behavior here? I'm not sure I follow purpose of this additional logic. In particular, what is the rationale for not performing a cast in these conditions?

It looks like you're trying to enable generic "cast to geometry or geography" functionality, and trying to avoid generating code for cast that would essentially be a no-op. Is there any reason to avoid generating this code?

I want to make sure we're not baking in assumptions that are tied to the OmniSci backend in the generic expression API, so if you could walk me through the reasoning here that would be helpful.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

It might help to add a unit test just for this functionality.

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 20, 2019

Author Collaborator

well I will try to explain me.

In this PR the cast operation for geospatial works from point, linestring, polygon or multipolygon to geometry or geography.

point, linestring, polygon and multipolygon have a attribute called geotype that specify it is a geometry or geography (also it could be None, that is treated as geometry)

so cast from geometry to geometry is the same to cast int to int ... so I am follow the same idea of if op.to.equals(arg.type()): ...

currently omniscidb doesn't support cast to geometry ... but I am treating that just inside the mapd backend.

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

Ok, makes a bit more sense now. Can you perform the comparison with op.to instead of looking at target_type? op.to is an instance of DataType as opposed to target_type which can be either a string or a DataType.

target_type.lower() in ('geometry', 'geography')
and (
arg.type().geotype == target_type
or (arg.type().geotype is None and target_type == 'geometry')

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 20, 2019

Member

Ok, makes a bit more sense now. Can you perform the comparison with op.to instead of looking at target_type? op.to is an instance of DataType as opposed to target_type which can be either a string or a DataType.

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

@cpcloud Ok .. I will change that! thanks!

@cpcloud cpcloud added this to the 1.2.0 milestone Jun 20, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 20, 2019

there are more bug to fix .. I am working on that

@xmnlab xmnlab force-pushed the Quansight:add-more-geo-funcs-mapd branch from 36c305b to d171051 Jun 21, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 21, 2019

@cpcloud it is done for a new review!

py35_test failed because a CondaHTTPError: HTTP 504 GATEWAY_TIMEOUT

to_geotype = op.to.geotype
if (
from_geotype == to_geotype
or (from_geotype is None and to_geotype == 'geometry')

This comment has been minimized.

Copy link
@cpcloud

cpcloud Jun 21, 2019

Member

I think this can be reduced to

from_geotype = arg.type().geotype or 'geometry'
to_geotype = op.to.geotype

if from_geotype == to_geotype:
   ...

This comment has been minimized.

Copy link
@xmnlab

xmnlab Jun 21, 2019

Author Collaborator

looks much better. thanks! I will change that

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

@xmnlab Can you rebase to pick up the fix for the failing tests?

@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Once this is green, I'll merge.

@xmnlab xmnlab force-pushed the Quansight:add-more-geo-funcs-mapd branch from 4fc33ed to 9eaf660 Jun 24, 2019

@xmnlab

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 24, 2019

rebased! thanks @cpcloud !

@cpcloud cpcloud merged commit 8427e9e into ibis-project:master Jun 24, 2019

15 checks passed

ci/circleci: python35_test Your tests passed on CircleCI!
Details
ci/circleci: python36_benchmark Your tests passed on CircleCI!
Details
ci/circleci: python36_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python36_docs Your tests passed on CircleCI!
Details
ci/circleci: python36_test Your tests passed on CircleCI!
Details
ci/circleci: python37_conda_build Your tests passed on CircleCI!
Details
ci/circleci: python37_test Your tests passed on CircleCI!
Details
codecov/patch 96.22% of diff hit (target 87.46%)
Details
codecov/project Absolute coverage decreased by -1.77% but relative coverage increased by +8.76% compared to f05fad2
Details
ibis-project.ibis Build #20190624.12 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py36) WindowsCondaBuild py36 succeeded
Details
ibis-project.ibis (WindowsCondaBuild py37) WindowsCondaBuild py37 succeeded
Details
ibis-project.ibis (WindowsTest py35) WindowsTest py35 succeeded
Details
ibis-project.ibis (WindowsTest py36) WindowsTest py36 succeeded
Details
ibis-project.ibis (WindowsTest py37) WindowsTest py37 succeeded
Details
@cpcloud

This comment has been minimized.

Copy link
Member

commented Jun 24, 2019

Thanks @xmnlab!

@cpcloud cpcloud self-assigned this Jun 24, 2019

@cpcloud cpcloud added this to In progress in OmniSci via automation Jun 24, 2019

@xmnlab xmnlab deleted the Quansight:add-more-geo-funcs-mapd branch Jun 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.