-
Notifications
You must be signed in to change notification settings - Fork 13
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
srtm stac fixes #261
srtm stac fixes #261
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-11T15:35:47Z I think there's an extra space in the link? could probably drop "proxy" as well |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-11T15:35:48Z should these installs be moved to their own cell like the science tutorials? |
The only other thing would be to maybe add the VEDA guidelines, mainly just fixing the heading size and capitalization, and adding an author - could be optional but might save you a PR down the road |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-13T22:44:39Z I think there's an extra space in the title between MAAP and STAC? Should the title be specific to the SRTM dataset or would it be okay to remove?
smk0033 commented on 2023-07-14T13:32:44Z Hmm, I'm not sure why it's coming up that way, when I edit the notebook there isn't an extra space smk0033 commented on 2023-07-14T13:40:08Z Oh wait, I see the extra space in the title, I thought the extra space was still showing up in the text that is hyperlinked. I'll change that! I think we could remove SRTM since it's in the description and will help shorten the title. |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-13T22:44:40Z Not sure we need an "about the data" for a technical tutorial, thoughts? Related, I'd like to see "Additional Resources" about visualizing instead of the dataset then smk0033 commented on 2023-07-14T13:36:36Z VEDA also has a section labeled "Approach" that they do in some of theirs. Maybe we could do that instead of "About the Data"? I think Brian and I decided to leave "approach" out of the science tutorials though since we had the brief description and you're walked through the steps throughout the tutorial. Switching up the additional resources would make sense too, so I can look for more about visualization. freitagb commented on 2023-07-14T20:38:37Z I think if we have reference to the SRTM dataset in the notebook, there should be some clarification within the notebook that it's not the authoritative data distributed by USGS, but transformed COGs within the MAAP STAC catalog. I agree that we can probably remove it from the top matter altogether if the purpose of the json is purely about visualizing a dataset within the MAAP STAC catalog. |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-13T22:44:41Z These subheadings seem a little small, maybe we can make them slightly bigger? smk0033 commented on 2023-07-14T13:37:24Z They look so small here, I think I used ### for the markdown size, but I can up them to ## like the other headers smk0033 commented on 2023-07-14T14:07:52Z Apparently the titles are showing up funky for some reason, this is already at ## heading size |
Hmm, I'm not sure why it's coming up that way, when I edit the notebook there isn't an extra space View entire conversation on ReviewNB |
VEDA also has a section labeled "Approach" that they do in some of theirs. Maybe we could do that instead of "About the Data"? I think Brian and I decided to leave "approach" out of the science tutorials though since we had the brief description and you're walked through the steps throughout the tutorial. Switching up the additional resources would make sense too, so I can look for more about visualization. View entire conversation on ReviewNB |
They look so small here, I think I used ### for the markdown size, but I can up them to ## like the other headers View entire conversation on ReviewNB |
Oh wait, I see the extra space in the title, I thought the extra space was still showing up in the text that is hyperlinked. I'll change that! I think we could remove SRTM since it's in the description and will help shorten the title. View entire conversation on ReviewNB |
Apparently the titles are showing up funky for some reason, this is already at ## heading size View entire conversation on ReviewNB |
@freitagb thoughts on Technical Tutorial guidelines? I included an "About the Data" section, but Jamison and I wonder if this can be left out? I know VEDA also has the option of doing an "Approach" section, but I think we decided to leave this out of the science tutorials since there is a brief description, and you're walked through the steps throughout the tutorial. |
I think if we have reference to the SRTM dataset in the notebook, there should be some clarification within the notebook that it's not the authoritative data distributed by USGS, but transformed COGs within the MAAP STAC catalog. I agree that we can probably remove it from the top matter altogether if the purpose of the json is purely about visualizing a dataset within the MAAP STAC catalog. View entire conversation on ReviewNB |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-18T18:02:09Z Something like "In this notebook, we visualize SRTM Cloud-Optimized GeoTIFFs (COGs) from MAAP's STAC using a generated mosaic from MAAP's TiTiler." to highlight the purpose of this notebook, thoughts? smk0033 commented on 2023-07-18T18:34:14Z I like something along the lines of that, definitely a little more descriptive! |
View / edit / reply to this conversation on ReviewNB jjfrench commented on 2023-07-18T18:02:10Z Can get rid of the "pip install" in the markdown cell since we're listing it down below in the code cell |
I like something along the lines of that, definitely a little more descriptive! View entire conversation on ReviewNB |
Oh sorry, my comment wasn't very clear -
Is fine, we just don't need the |
Gotcha! I just removed it all since they're in the code cell now, but I'll put that little section back! 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚀
(Sorry, this should be under this PR but git was being weird: #253)
Reference ticket: #237
Tutorial got moved to STAC, updated the endpoint and made minor adjustments.