-
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
FEAT: Add geopandas as output for omniscidb #1858
FEAT: Add geopandas as output for omniscidb #1858
Conversation
|
@ian-r-rose I am adding new features on postgis implementation. if you have a time, could you take a look into that? |
Codecov Report
@@ Coverage Diff @@
## master #1858 +/- ##
=========================================
Coverage ? 85.68%
=========================================
Files ? 81
Lines ? 15537
Branches ? 1994
=========================================
Hits ? 13313
Misses ? 1856
Partials ? 368
|
|
@cpcloud it is done for review :) thanks! |
703e488
to
cffc14a
Compare
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.
@cpcloud it is done for a new review! thanks!
|
@cpcloud when you have time could you review this one? so I can move forward with the remains PR |
|
Reviewing now. |
|
thanks @cpcloud for the feedback! I will work on that! |
5d5e9be
to
6194340
Compare
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.
@cpcloud thanks for the review. I applied the suggestion you made. it is done for a new review!
ibis/mapd/client.py
Outdated
| """ | ||
|
|
||
| def to_df(self): | ||
| if not isinstance(self.cursor, Cursor): |
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!
ibis/mapd/client.py
Outdated
| def to_df(self): | ||
| if not isinstance(self.cursor, Cursor): | ||
| if self.cursor is None: | ||
| return geopandas.GeoDataFrameDataFrame([]) |
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.
thanks for catch that up!
|
@cpcloud, when you have time, could you review again this PR? Thanks! |
6194340
to
95ec563
Compare
|
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.
LGTM! Merging! Thanks @xmnlab
95ec563
to
263255c
Compare
|
@cpcloud rebase done! but it seems it is raising some errors. |
|
@xmnlab Can you rebase and push again to pick up fixes and resolve the conflicts? |
263255c
to
e5995a3
Compare
|
|
@cpcloud do you have any idea about these errors? |
|
@xmnlab Looking now. Looks like there's a conflict again here, sorry about that. Can you fix up and push again? |
440b339
to
59d2d2c
Compare
|
rebased! :) |
|
it seems the errors related to |
|
@xmnlab We've merged some code in the meantime that uses the old version of |
|
After that, this is good to merge. |
|
thanks @cpcloud I didn't realize that the problem could be in new files. |
|
560d677
to
0088881
Compare
|
@cpcloud it is done for a new review! thanks! |
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.
LGTM, merging!
In this PR: