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

CRS.__eq__ crash on inconsistent types #1439

Closed
drnextgis opened this issue Aug 14, 2018 · 6 comments
Closed

CRS.__eq__ crash on inconsistent types #1439

drnextgis opened this issue Aug 14, 2018 · 6 comments

Comments

@drnextgis
Copy link
Contributor

drnextgis commented Aug 14, 2018

We are using rasterio in telluric library and got the following issue: satellogic/telluric#123. The root cause of this issue is that CRS.__eq__ crashes on inconsistent types. What do you think about it?

@vincentsarago
Copy link
Member

👋 @drnextgis this is expected, Sean also mentioned that from rasterio 1.0 CRS equality should only works between CRS object (#1357).

CRS are object, you cannot compare an object with an int in my opinion so raising an error is the expected behavior and masking it could be dangerous.

Hope this answer is 👌.

@vincentsarago
Copy link
Member

@drnextgis can we close here ?

@drnextgis
Copy link
Contributor Author

@vincentsarago thanks for reply.

@a-gn
Copy link
Contributor

a-gn commented Feb 12, 2022

👋 @drnextgis this is expected, Sean also mentioned that from rasterio 1.0 CRS equality should only works between CRS object (#1357).

CRS are object, you cannot compare an object with an int in my opinion so raising an error is the expected behavior and masking it could be dangerous.

Is it really necessary to raise an exception here? Throughout Python, equality between objects of different types is implemented and returns normally, for example:

>>> 3 == "a"
False

This crashes my code, because I configured my ORM (SQLAlchemy) to store a CRS as a WKT string, and convert it back to a rasterio CRS object when loading data.

When the database entry is NULL, that leads SQLAlchemy to compare a CRS with symbol('NO_VALUE'), which causes this error:

  File "/home/agobbin/venv/all3.9/lib/python3.9/site-packages/sqlalchemy/sql/type_api.py", line 490, in compare_values
    return x == y
  File "/home/agobbin/venv/all3.9/lib/python3.9/site-packages/rasterio/crs.py", line 89, in __eq__
    other = CRS.from_user_input(other)
  File "/home/agobbin/venv/all3.9/lib/python3.9/site-packages/rasterio/crs.py", line 492, in from_user_input
    return cls.from_epsg(value)
  File "/home/agobbin/venv/all3.9/lib/python3.9/site-packages/rasterio/crs.py", line 333, in from_epsg
    obj._crs = _CRS.from_epsg(code)
  File "rasterio/_crs.pyx", line 330, in rasterio._crs._CRS.from_epsg
OverflowError: value too large to convert to int

I think SQLAlchemy is making the reasonable assumption that equality will always return something, because the Python docs say that:

A rich comparison method may return the singleton NotImplemented if it does not implement the operation for a given pair of arguments. By convention, False and True are returned for a successful comparison. However, these methods can return any value, so if the comparison operator is used in a Boolean context (e.g., in the condition of an if statement), Python will call bool() on the value to determine if the result is true or false.

So even though the docs don't explicitly say "raising an exception for inconsistent types is forbidden", they imply that a comparison between inconsistent types returns False or NotImplemented.

Equality is often implemented by first checking type consistence, then comparing the values if types are consistent. I think this would be more sensible, because crashing in __eq__ is not the norm and risks surprising users (at least two of us :) ).

Of course, this means that some logic error will be silenced, but that's a design decision by the Python devs, and it's (as far as I can tell) the norm in all common Python libraries, as well as in the standard library.

@snowman2
Copy link
Member

Related: #1754

@a-gn
Copy link
Contributor

a-gn commented Feb 12, 2022

I see, the problem is that this code only catches CRSError, but from_user_input can raise other exception types.

For example, this code:

from rasterio.crs import CRS
CRS.from_epsg(4326) == 1111111111111111111

raises an OverflowError:

Traceback (most recent call last):
  File "crs_eq.py", line 2, in <module>
    CRS.from_epsg(4326) == 1111111111111111111
  File "/Users/arno/venv/all3.8/lib/python3.8/site-packages/rasterio/crs.py", line 89, in __eq__
    other = CRS.from_user_input(other)
  File "/Users/arno/venv/all3.8/lib/python3.8/site-packages/rasterio/crs.py", line 492, in from_user_input
    return cls.from_epsg(value)
  File "/Users/arno/venv/all3.8/lib/python3.8/site-packages/rasterio/crs.py", line 333, in from_epsg
    obj._crs = _CRS.from_epsg(code)
  File "rasterio/_crs.pyx", line 330, in rasterio._crs._CRS.from_epsg
OverflowError: value too large to convert to int

Should I open a new issue for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants