Skip to content
This repository has been archived by the owner on Jan 4, 2022. It is now read-only.

ISS 777: Fix hardcoded TmsImage url #789

Merged
merged 6 commits into from
Oct 30, 2019
Merged

ISS 777: Fix hardcoded TmsImage url #789

merged 6 commits into from
Oct 30, 2019

Conversation

mgregor-oa
Copy link
Collaborator

@mgregor-oa mgregor-oa commented Oct 30, 2019

I updated the functionality of the class. It now requires you to pass a url to the class. I have removed all reference to mapbox and access_tokens. This should now work for any tms service passed to it.

url = r"https://a.tile.openstreetmap.org/{z}/{x}/{y}.png"
img = TmsImage(url=url, zoom=18)
img2 = TmsImage(url=url, zoom=18, bbox=bbox)
aoi = img.aoi(bbox=bbox)
print(img)
print(aoi)
print(img2)
aoi.plot(), img2.plot()

@mgregor-oa mgregor-oa requested a review from drwelby October 30, 2019 16:48
@mgregor-oa mgregor-oa changed the title Passes test vs my working venv ISS 777: Fix hardcoded TmsImage url Oct 30, 2019
@drwelby
Copy link
Contributor

drwelby commented Oct 30, 2019

Tests pass locally for me

self._name = "image-{}".format(str(uuid.uuid4()))
self._url_template = url + "?access_token={token}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're no longer doing templating let's get rid of ._url_template and just make it ._url

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good to me. I wanted to edit as little as possible, but that change makes sense to me.

Copy link
Contributor

@drwelby drwelby left a comment

Choose a reason for hiding this comment

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

Can you also update the section in docs/imagery_access.rst?


Instead of an ID the zoom level to use can be specified (default is 22). Changing the zoom level will change the resolution of the image. Note that different image sources are used at different zoom levels.
Instead of an ID the zoom level to use can be specified (default is 18). Changing the zoom level will change the resolution of the image. Note that different image sources are used at different zoom levels.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can drop the part about "Different image sources are used at different zoom levels

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I was using a DG test that does this, but depending on the service this isn't applicable.

bbox (list): (optional) Bounding box of AOI, if aoi() method is not used.

Example:
>>> img = TmsImage(zoom=13, bbox=[-109.84, 43.19, -109.59, 43.34])'''
>>> img = TmsImage(url=r"https://earthwatch.digitalglobe.com/earthservice/tmsaccess/tms/1.0.0/DigitalGlobe:ImageryTileService@EPSG:3857@jpg/{z}/{x}/{y}.jpg?connectId=", zoom=13, bbox=[-109.84, 43.19, -109.59, 43.34])'''
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to use url= here since it's positional and required

@@ -66,7 +68,8 @@ def test_tms_image_aoi(self):
# a 1 x 4 chunk of tiles, so 256 x 1024 pixels
# then offset by 1/2 tile in x and y
bbox = [-74.71046447753906, 40.53624234037728, -74.70909118652344, 40.54041698756514]
img = self.gbdx.tms_image(zoom=18)
url = r"https://a.tile.openstreetmap.org/{z}/{x}/{y}.png"
img = TmsImage(url=url, zoom=18)
Copy link
Contributor

Choose a reason for hiding this comment

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

same in here with url=

@drwelby drwelby merged commit 4173608 into dev Oct 30, 2019
@drwelby
Copy link
Contributor

drwelby commented Oct 30, 2019

Looks good, thanks for the fix

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

Successfully merging this pull request may close these issues.

2 participants