titiler-pgstac in R#491
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@wildintellect @smk0033 @hrodmn @zacdezgeo can someone please review this notebook when you find time. I want to make the changes and close the ticket before I leave for my break. Thank you |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-05-02T11:22:44Z If you just need the XYZ tile layer URL I would not reformat the WMTS url to get it - just use |
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
@HarshiniGirish this looks really good! My only request is that you update the last section to use ak_tilejson$tiles instead of gsub-ing the WMTS url to get the XYZ tile URL.
|
🤔 do we want to make sure we can point people to WMTS urls if they wanted to use the tiles in QGIS? I think that would go in a different doc. I agree stick to simpler XYZ for Leaflet maps. |
|
View / edit / reply to this conversation on ReviewNB wildintellect commented on 2025-05-02T22:42:41Z Switch to |
|
View / edit / reply to this conversation on ReviewNB wildintellect commented on 2025-05-02T22:42:42Z Line #7. coords <- matrix(c( I think this can be simplified by using st_bbox
https://gis.stackexchange.com/a/485194
which means the center can also be obtained with st_centroid |
|
@hrodmn @wildintellect thank you for the initial review, I have made the changes. Please let me know if anything has to be implemented going forward. |
|
@hrodmn I assigned final review to you. |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-10T02:18:17Z Line #4. response <- GET(url, timeout(10)) since you are using httr2 now, you need to update this to use httr2::request instead of GET - https://httr2.r-lib.org/ |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-10T02:18:17Z Line #2. collection_tilejson <- fromJSON(collection_tilejson, simplifyVector = TRUE) We are skipping an important step here. To get the XYZ tile URL you need to send a request to the tilejson endpoint. You can get that link in a similar way to how you have it set up below: tilejson_url <- collection_info$links[collection_info$links$rel == "tilejson", "href"]
tilejson_url <- gsub("\\{tileMatrixSetId\\}", "WebMercatorQuad", tilejson_url)
collection_tilejson <- request(tilejson_url)
...
|
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-10T02:18:18Z Line #5. wmts_url <- ak_search_request$links[ak_search_request$links$rel == "wmts", "href"] I know it is confusing how many different links are stuffed in here but we should just get the tilejson link here, then pull the tiles link from the tilejson response. |
|
Thanks for the review @hrodmn. I have made all the suggested changes. Please have a look at your convenience. |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-14T20:12:43Z Line #7. rescale = paste(agb_raw$rescale[1, ], collapse = ",") # e.g., "0,400" can you add |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T10:48:31Z Line #8. colormap_name = agb_raw$colormap_name also need to add |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T10:48:32Z Line #22. lng = st_coordinates(center)[1], you will need to set this to |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T10:48:32Z this whole cell can be deleted - you already requested the tilejson in the above cell |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T10:48:33Z Line #25. setView(lng = center_lng, lat = center_lat, zoom = 6) change this to
the existing values of |
|
Thanks for making those changes @HarshiniGirish! I found a few more things that need to be addressed before we merge. You will also need to add an entry for the new notebook to It would be really helpful if you could make sure the notebook can be cleanly rendered from top to bottom (run all cells) after restarting the kernel - that is what is going to happen when we merge the PR and rebuild the docs site so we need to make sure that each notebook can be rendered cleanly without any errors! |
|
Thank you @hrodmn. Please let me know if anything else has to be addressed! |
|
@HarshiniGirish the notebook will not render for me with a fresh kernel - need to define center_lng and center_lat before using them in the first leaflet map |
|
My bad. missed it earlier. I made the update, please try again @hrodmn |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T15:16:06Z Line #25. lng = center_lng, this variable is not defined yet! |
|
thank you @hrodmn made the change. Not sure it is what you were looking for. I apologize for the disorder here |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T17:54:57Z If you make this a level 1 header instead of a level 2 header, it will be used for the article's title in the docs website |
|
View / edit / reply to this conversation on ReviewNB hrodmn commented on 2025-07-16T17:54:58Z Please add a space between the name and affiliation (e.g. Harshini Girish (UAH)) |
docs/source/technical_tutorials/working_with_r/visualizing with titiler-pgstac.ipynb
Show resolved
Hide resolved
|
Thanks for your patience with my review @HarshiniGirish! I found a few small things at the very top but the notebook renders beautifully now without any errors. Once you take care of the the markdown edits at the top and rename the notebook without spaces I'll approve it and you can merge this PR 🚀 |
|
Thank you @hrodmn! made the updates! |
hrodmn
left a comment
hrodmn
left a comment
There was a problem hiding this comment.
Looks great - thanks @HarshiniGirish
3 participants
No description provided.