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

commit some tweaks made in advance of the agu demo #31

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jjmcnelis
Copy link
Member

I made some code improvements + simplifications to the SWOT-EA-2021 notebook inside this branch. I had to make edits before recording parts of it for AGU, and some of the tweaks are worth merging into the main branch IMO.

I added a few more cells to the end of the workflow to compare downloads speeds for two equivalent L4 MUR subsets accessed from our s3 storage (netCDF) as well as the aws open reg (Zarr). I was sorta surprised by the result; comparable download times over the admittedly very small time/space coverage. Still neat to see tho.

@jjmcnelis jjmcnelis self-assigned this Dec 22, 2021
@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
Contributor

@ScienceCat18 ScienceCat18 left a comment

Choose a reason for hiding this comment

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

Thanks for these updates Jack. My comments based on the review of the ReviewNB diff app (very cool and helpful tool!!):

  • I would keep the in-cell comments from the original notebook, I think these are helpful and it's good form to have comments in code
  • RESULTS section added at the end, a very nice addition to see the time/compute processing for the two cases. However, I would recommend adding a sentence stating this following Results section is optional to the workflow, and is meant as exploratory tool to compare downloads speeds for two equivalent L4 MUR subsets accessed from the PODAAC s3 storage (netCDF) in Earthdata Cloud in AWS, as well as the AWS Registry of Open Data MUR version (Zarr). Basically just add a bit more context for the notebook user.
  • can you cluster the imports at the top and add brief comments what each import or cluster is used for? e.g. plotting, data access in s3, etc etc. an example is here https://nasa-openscapes.github.io/2021-Cloud-Workshop-AGU/tutorials/04_On-Prem_Cloud.html
    Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants