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

cop dem example notebook #1

Closed
wants to merge 8 commits into from
Closed

cop dem example notebook #1

wants to merge 8 commits into from

Conversation

rbavery
Copy link

@rbavery rbavery commented Jan 5, 2023

@wildintellect I've added an example that shows how to handle multiple tiles with stackstac, including a section on masking missing values to make a mean composite. Ready for your review!

@rbavery rbavery requested a review from wildintellect January 5, 2023 22:43
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

@wildintellect wildintellect left a comment

Choose a reason for hiding this comment

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

Feedback on ReviewNB

Copy link

Made comments on notebook.

@rbavery rbavery changed the title Cop dem cop dem example notebook Jan 9, 2023
@rbavery rbavery requested a review from wildintellect January 11, 2023 03:12
@wildintellect
Copy link

@rbavery should we have @kathrynberger review the notebook too for a different perspective? perhaps also attempt to run it on SageMaker?

@wildintellect
Copy link

Looks good @rbavery now I just need to think about where to stash these for now and talk to AWS about how to have dev docs to match our dev environment.

Copy link

@kathrynberger kathrynberger left a comment

Choose a reason for hiding this comment

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

This notebook looks really sleek, great job on it! It covers each process step-by-step and is easy to follow and interpret. I'm requesting changes only for two minor components, otherwise it is good to go.

  1. Minor revision/recommendation - might be helpful to zoom out further with the Leafmap basemap image. I understand that is to help provide context to where this DEM is presented - but it doesn't show the river that you're trying to demonstrate in your ultimate product/DEM map. My recommendation would be to zoom out so you can show the same scale/river you are trying to detect - you might even highlight that that is the what you will be exploring in this example, so that it is all tied together. That way, it gives context to the DEM map and what we are looking for. (Otherwise the DEM map, shows a river, but we have a hard time placing where it is on the map of Belize).
  2. Only other question - is this notebook meant to be run on SageMaker? I've never used it and am unfamiliar. I was seeking out documentation within the rest of the directory (which I realize is outside of this PR) and found it within the CMIP6 example, perhaps it would make sense to highlight the the order of operations (must follow set up instructions on notebook 1, to run on other nbs, etc) or separate these instructions to the main read me files instead so the process is a bit clearer.

@kathrynberger
Copy link

  1. Only other question - is this notebook meant to be run on SageMaker? I've never used it and am unfamiliar. I was seeking out documentation within the rest of the directory (which I realize is outside of this PR) and found it within the CMIP6 example, perhaps it would make sense to highlight the the order of operations (must follow set up instructions on notebook 1, to run on other nbs, etc) or separate these instructions to the main read me files instead so the process is a bit clearer.

Following from conversation with @wildintellect, testing it on SageMaker is not part of the PR. Now that I've got AWS credentials and access to SageMaker, I can confirm that the notebook looks great. Easy to follow.

I only encountered one error after installing jupyter-leaflet from the extensions. Even after restarting Juypter or refreshing the SageMaker nb, I still received an Error displaying widget: model not found and so was not able to display the map that had previously worked when I reviewed this PR locally.

@kathrynberger
Copy link

Following up on the error received by SageMaker re: leafmap (and my follow-up conversation with @wildintellect) here is the identical error received with ipyleaflet. See here and here.

@wildintellect wildintellect added the documentation Improvements or additions to documentation label Jan 19, 2023
@rbavery
Copy link
Author

rbavery commented Jan 24, 2023

@kathrynberger the widget needs to be installed from the jupyter extension manager, I think this is detailed in the initial notebook section in the other notebook, I'll copy it over.

also, those errors linked are for Sagemaker Studio Lab which is different from Sagemaker Studio (confusing!). Sagemaker Studio allows you to customize the environment more, Studio Lab does not I think.

@kathrynberger
Copy link

@kathrynberger the widget needs to be installed from the jupyter extension manager, I think this is detailed in the initial notebook section in the other notebook, I'll copy it over.

@rbavery I had installed it from the jupyter extension manager, and both restarted the kernel and refreshed the jupyter notebook all to no avail. Were there any other additional steps used?

also, those errors linked are for Sagemaker Studio Lab which is different from Sagemaker Studio (confusing!). Sagemaker Studio allows you to customize the environment more, Studio Lab does not I think.

Ahh, I didn't realize the difference - you're right, the two are too closely named! Thanks for the heads up.

@wildintellect
Copy link

This is waiting on us figuring out where the final home of the notebook should be. A new ticket should be opened for any improvements or porting to SageMakerStudioLab.

@rbavery
Copy link
Author

rbavery commented Feb 7, 2023

@wildintellect I think this issue already exists: https://github.com/developmentseed/aws-asdi/issues/70

@wildintellect
Copy link

Closing in favor of developmentseed/asdi-examples#2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants