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

srtm stac fixes #261

Merged
merged 10 commits into from
Jul 19, 2023
Merged

srtm stac fixes #261

merged 10 commits into from
Jul 19, 2023

Conversation

smk0033
Copy link
Contributor

@smk0033 smk0033 commented Jul 10, 2023

(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.

@smk0033 smk0033 added bug Something isn't working documentation Improvements or additions to documentation labels Jul 10, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

This was referenced Jul 10, 2023
@review-notebook-app
Copy link

review-notebook-app bot commented Jul 11, 2023

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


@review-notebook-app
Copy link

review-notebook-app bot commented Jul 11, 2023

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?


@jjfrench
Copy link
Contributor

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

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 13, 2023

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.

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 13, 2023

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.

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 13, 2023

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

Copy link
Contributor Author

smk0033 commented Jul 14, 2023

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

Copy link
Contributor Author

smk0033 commented Jul 14, 2023

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

Copy link
Contributor Author

smk0033 commented Jul 14, 2023

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

Copy link
Contributor Author

smk0033 commented Jul 14, 2023

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

Copy link
Contributor Author

smk0033 commented Jul 14, 2023

Apparently the titles are showing up funky for some reason, this is already at ## heading size


View entire conversation on ReviewNB

@smk0033
Copy link
Contributor Author

smk0033 commented Jul 14, 2023

@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.

Copy link

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

@wildintellect wildintellect requested review from freitagb and removed request for emileten July 17, 2023 20:36
@review-notebook-app
Copy link

review-notebook-app bot commented Jul 18, 2023

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!

@review-notebook-app
Copy link

review-notebook-app bot commented Jul 18, 2023

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


Copy link
Contributor Author

smk0033 commented Jul 18, 2023

I like something along the lines of that, definitely a little more descriptive!


View entire conversation on ReviewNB

@jjfrench
Copy link
Contributor

Oh sorry, my comment wasn't very clear -
I think

### Requirements

To be able to run this notebook you'll need the following requirements:

- rasterio
- folium
- requests
- tqdm
- rio-tiler (2.0b8) (Optional)
- cogeo-mosaic (Optional)

Is fine, we just don't need the pip install statements at the bottom of the markdown cell. Either way seems fine though!

@wildintellect wildintellect mentioned this pull request Jul 18, 2023
9 tasks
@smk0033
Copy link
Contributor Author

smk0033 commented Jul 19, 2023

Gotcha! I just removed it all since they're in the code cell now, but I'll put that little section back! 🙂

Copy link
Contributor

@jjfrench jjfrench left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

@smk0033 smk0033 merged commit fcd1dab into develop Jul 19, 2023
@smk0033 smk0033 deleted the srtm-stac-fixes branch July 19, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants