-
Notifications
You must be signed in to change notification settings - Fork 590
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
[MapD] Added Geospatial functions #1678
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1678 +/- ##
==========================================
- Coverage 90% 87.49% -2.52%
==========================================
Files 186 188 +2
Lines 27486 27701 +215
Branches 2344 2351 +7
==========================================
- Hits 24739 24236 -503
- Misses 2338 3055 +717
- Partials 409 410 +1
|
|
@xmnlab Please add more tests. Then it'll be easier to see whether the API is convenient enough or still needs some tuning. In general it looks good though. |
|
@kszucs That sounds good, thanks for the feedback. I will work on that today. |
|
@cpcloud @kszucs I am adding a new table for geo data (https://github.com/ibis-project/ibis/blob/c4572177b45420343b2580396460e9e5ca8f66fd/ci/ibis-testing-data/geo.csv) it seems is there another way to do that and avoid using a "external" storage? maybe creating a new repo just for data on ibis-project. |
|
@cpcloud @kszucs any thoughts about the previous comment (#1678 (comment)) ? |
|
it seems download function tries to get data files from https://storage.googleapis.com/ibis-testing-data, is it right ? how can I save geo.csv there? is there another way to do that and avoid using a "external" storage? maybe creating a new repo just for data on ibis-project. |
|
@xmnlab please rebase |
05da7b2
to
f1271c0
Compare
ci/datamgr.py
Outdated
| # get data for geospatial support tests | ||
| geo_url = ( | ||
| 'https://raw.githubusercontent.com/Quansight/ibis/' | ||
| 'geospatial_functions/ci/ibis-testing-data/geo.csv' |
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.
Please create a PR to https://github.com/ibis-project/testing-data with the geo.csv added and change the download script to fetch https://api.github.com/repos/ibis-project/testing-data/tarball/master
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.
@kszucs that sounds pretty good .. I am going to do that now 👍
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.
PR opened ibis-project/testing-data#1
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.
I will now change download function
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.
Merged :)
|
@xmnlab Please rebase. Is this PR still work in progress? |
|
@kszucs OK I am going to rebase that now. thanks! |
3e0fa42
to
72a8990
Compare
|
code rebased. |
560386c
to
47abb55
Compare
|
hey @kszucs I think I will work on these 3 functions in a new PR. I will remove the on the title |
|
@kszucs is it possible to remove the jobs on circle ci related to python35? |
|
@xmnlab I think those jobs are stuck, because this PR was created before their removals. |
|
oh I see .. thanks for the explanation @kszucs I think it is done for a review 👍 |
|
First of all, We're going to need more tests. Please improve the coverage. |
|
@kszucs sounds good. I am working on that now! thanks! |
4ce593d
to
41be8dc
Compare
|
hey @kszucs I added tests for each function .. I just need to fix 3 tests/functions (tomorrow) ... if you have any tip about improving this test code coverage let me know. thanks! |
ibis/tests/all/test_geospatial.py
Outdated
|
|
||
|
|
||
| @tu.skipifnot_backend(MapD) | ||
| def test_geo_op(backend, geo): |
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.
@xmnlab Could You decompose and/or pytest.parametrize it to multiple test functions?
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.
sounds good @kszucs . thanks for the feedback. I am working on that.
|
@kszucs it seems codecov now is green :) I think it is ok for a new review :) |
|
@kszucs thanks for the feedback! I am going to work on that now :) |
6ec0c65
to
d2ac169
Compare
d2ac169
to
4c3969c
Compare
|
@kszucs I made the changes you suggested. let me know any thought about the last changes :) |
|
@kszucs I am back! thansk for the commit! do you already know about |
|
@xmnlab I gave You the wrong github url, sorry for that :) |
|
it seems that impyla v0.14.2.2 (latest release, 28 days ago) doesn't use if is not possible to ask for a new release there, maybe would be good to keep the url for the commit as you have made before. |
|
after this PR be merged I will create an issue about the improvement of documentation about geo spatial data types and functions with explanation and examples. |
|
not sure if this is relevant for this PR but pymapd v0.8.0 should be available in some minutes (conda-forge/pymapd-feedstock#30) |
|
Cool, thanks @xmnlab! |
|
@kszucs do you think it is ready to be merged? |
|
It is green so yes! Thanks for the hard work @xmnlab! |
|
thank you so much @kszucs for all the help! |
This PR solves #1665 and solves #1707
Add Geo Spatial functions on the main structure and define these functions inside MapD backend.
References:
Depends on #1666 ( PR 1666 was used as base for the current PR)
Geospatial functions
ST_Transform (Returns a new geometry with its coordinates transformed to a different spatial reference system.)ST_SetSRID (Sets the SRID on a geometry to a particular integer value.)CastToGeographyTODO: will be added in a new PR.