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: numpy floats parsed incorrectly #955

Merged
merged 2 commits into from
Nov 7, 2024
Merged

Conversation

juhi24
Copy link
Contributor

@juhi24 juhi24 commented Nov 6, 2024

Referring to SciTools/cartopy#2472 (comment)

Currently owslib cannot handle bbox defined as numpy.float64. This PR fixes the referred issue by using str instead of repr.

@geographika
Copy link
Contributor

Thanks @juhi24. The CI looks like it is failing on unrelated issues.

It looks like this line was originally request['bbox'] = ','.join([str(x) for x in bbox]) but was updated to use repr in
c9e2ddb - the reason being "changing str to repr to maintain precision in bbox coords".

It would be good to add a test with a high precision bbox values to check if str strips values off the end.

Also linking to a related issue #878.

@juhi24
Copy link
Contributor Author

juhi24 commented Nov 6, 2024

Hi, @geographika. I cannot reproduce the difference in precision in the earliest and latest supported Python versions 3.10 or 3.13, nor in legacy 3.6. All give:

>>> f = 123.4567890123456789
>>> str(f)
'123.45678901234568'
>>> repr(f)
'123.45678901234568'

I think this was a Python 2 issue:

>>> f = 123.4567890123456789
>>> str(f)
'123.456789012'
>>> repr(f)
'123.45678901234568'

The c9e2ddb was committed in 2011.

@geographika
Copy link
Contributor

@juhi24 - thanks for checking this, I'm +1 on merging.

It would be good to add in a test just confirming the precision for a bbox is retained. I think adding a simple unit test for __build_getmap_request (for WMS 1.3.0) to test_wms_getmap.py would work, without needing to make an actual request to a server. If you're able to add this to the pull request that would be great.

@juhi24
Copy link
Contributor Author

juhi24 commented Nov 7, 2024

@geographika, test added and passing.

@geographika
Copy link
Contributor

Just for future reference a float is truncated when converting with str, but in Python3 it is rounded to the same number of decimal places as repr so functionality will be unchanged with this pull request. To have higher accuracy use decimals - this approach would not have been possible using repr.

>>> f = 123.4567890123456789
>>> str(f)
'123.45678901234568'
>>> repr(f)
'123.45678901234568'

# to have higher precision use decimals - str converts correctly
>>> str(d)
'123.4567890123456805895330035127699375152587890625'
>>> repr(d)
"Decimal('123.4567890123456805895330035127699375152587890625')"
# this would fail to create a bbox

Thanks for the test and fix @juhi24.

@geographika geographika merged commit cdd92cd into geopython:master Nov 7, 2024
0 of 3 checks passed
@juhi24 juhi24 deleted the floats branch November 7, 2024 18:07
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

Successfully merging this pull request may close these issues.

2 participants