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

fix: Change map to geo_equals #2956

Merged
merged 6 commits into from
Sep 11, 2021
Merged

fix: Change map to geo_equals #2956

merged 6 commits into from
Sep 11, 2021

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Sep 10, 2021

This PR fix the problem with geo_equals.

Currently, equals operation is mapped to geo_equals and it is given problems as reported at #2197

Fix #2197

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

tests are failing; also pls add a release note

@pytest.mark.xfail_unsupported
def test_geo_equals(backend, geo):
"""
Test geo_equals/equals.
Copy link
Contributor

Choose a reason for hiding this comment

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

reference the issue in a comment. the Notes section is not needed here

@jreback jreback added the postgres The PostgreSQL backend label Sep 10, 2021
@cpcloud cpcloud added the geospatial Geospatial related functionality label Sep 10, 2021
@jreback jreback added this to the Next release milestone Sep 10, 2021
@xmnlab
Copy link
Contributor Author

xmnlab commented Sep 10, 2021

@jreback thanks for the review.

I added the release note and also changed 2 tests that were using .equals. to geo_equals.
In ibis, as I understood, equals usually returns a boolean, not an ibis expression.

I also updated the documentation about .equals

@xmnlab xmnlab marked this pull request as ready for review September 10, 2021 15:16
Copy link
Contributor

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

lgtm

assert expr.execute().all()

expr = geotable.geo_point.geo_equals(geotable.geo_linestring)
assert not expr.execute().all()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want all them to be different? Then any should be better I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense. I will change it now. thanks @datapythonista

@jreback jreback merged commit e6df814 into ibis-project:master Sep 11, 2021
@jreback
Copy link
Contributor

jreback commented Sep 11, 2021

thanks @xmnlab

@cpcloud cpcloud removed this from the Next release milestone Jan 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geospatial Geospatial related functionality postgres The PostgreSQL backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mutate geo column causes TypeError: geo_equals() got an unexpected keyword argument 'cache'
4 participants