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

Quickstart guide, keyframing, small updates #14

Merged
merged 31 commits into from
Jun 7, 2024
Merged

Conversation

ifranda
Copy link
Collaborator

@ifranda ifranda commented May 31, 2024

Adding two new notebooks

Copy link

github-actions bot commented May 31, 2024

👋 Thanks for opening this PR! The Cookbook will be automatically built with GitHub Actions. To see the status of your deployment, click below.
🔍 Git commit SHA: 46053bb
✅ Deployment Preview URL: https://ProjectPythia.github.io/vapor-python-cookbook/_preview/14

@ifranda ifranda marked this pull request as ready for review May 31, 2024 20:19
@ifranda ifranda requested a review from NihanthCW May 31, 2024 20:19
@ifranda
Copy link
Collaborator Author

ifranda commented May 31, 2024

Any suggestions are appreciated!

For downloading the datasets and session files, I included links to the files in Google Drive. Let me know if there is a better way!

Copy link
Collaborator

@NihanthCW NihanthCW left a comment

Choose a reason for hiding this comment

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

Thanks @ifranda. Looks good. A few suggestions:

  • In the quickstart and other new notebooks. Can downloading the dataset be done as a cell int he notebook itself to prevent them from having to download it separately? What do you think?

@ifranda
Copy link
Collaborator Author

ifranda commented Jun 3, 2024

I think we'd be able to! Is the dataset too big to host on GitHub? That would probably be the easiest because then they'd have it when they clone the repository. It's about 170 MB. Otherwise I think we could find another way!

@NihanthCW
Copy link
Collaborator

RDA would be a good option. I've moved VAPOR sample data to RDA. We can move Katrina data to the same place?

Copy link
Collaborator

@NihanthCW NihanthCW left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks Ian.
Two suggestions:

  • The download is working great! It might be good to check if the file already exists so that people are not re-downloading the file every time they run the notebook.
  • How do you feel about including one cell to allow people rotate and look around in the scene just before exporting the image? Something similar to viz = VaporVisualizerWidget(ses)

@ifranda
Copy link
Collaborator Author

ifranda commented Jun 5, 2024

Good ideas! Another thought -- if users are following along by copying and pasting into their own workspace instead of cloning the whole repository, they won't have access to the get_data.py script. Another idea I had was to have a cell for command lines, like this:

!curl -o Katrina.zip "https://data.rda.ucar.edu/ds897.7/Katrina.zip"
!unzip -o Katrina.zip -d ./data
!rm Katrina.zip

@NihanthCW
Copy link
Collaborator

Agreed.

@ifranda
Copy link
Collaborator Author

ifranda commented Jun 5, 2024

Ok, I've made some updates and I think it runs better!

@ifranda
Copy link
Collaborator Author

ifranda commented Jun 6, 2024

I got the image renderer working with a fresh conda build! But it looks like Natural Earth is the only builtin map at the moment, and I think that one only renders land, unless I'm wrong about that? Do you know if there's a way to get the oceans to render? Otherwise maybe we could try adding BigBlueMarble to the builtin maps

This is what it looks like at the moment:
image

Using the filepath to BigBlueMarble:
image

@NihanthCW
Copy link
Collaborator

You are right. The oceans are black in the dataset. Could this be another file that we could download? Alternatively, the black ocean down't look bad too...
I believe we intentionally donot include blue marble to avoid making the maps package too large.
Let's chat tomorrow.

@NihanthCW NihanthCW merged commit ea91930 into main Jun 7, 2024
2 checks passed
github-actions bot pushed a commit that referenced this pull request Jun 7, 2024
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.

2 participants