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

Add tutorial for visualizing volume data with Python, update docs #5535

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

nilsvu
Copy link
Member

@nilsvu nilsvu commented Oct 4, 2023

Proposed changes

Upgrade instructions

Code review checklist

  • The code is documented and the documentation renders correctly. Run
    make doc to generate the documentation locally into BUILD_DIR/docs/html.
    Then open index.html.
  • The code follows the stylistic and code quality guidelines listed in the
    code review guide.
  • The PR lists upgrade instructions and is labeled bugfix or
    new feature if appropriate.

Further comments

@nilsvu nilsvu requested a review from yoonso0-0 October 4, 2023 00:16
@nilsvu nilsvu force-pushed the tutorial_vis_python branch 2 times, most recently from 17f7131 to 079aa1f Compare October 5, 2023 05:32
@nilsvu nilsvu changed the title Add tutorial for visualizing volume data with Python Add tutorial for visualizing volume data with Python, update docs Oct 5, 2023
@nilsvu nilsvu force-pushed the tutorial_vis_python branch 2 times, most recently from 507a43b to 5163f53 Compare October 11, 2023 17:04
@nilsvu nilsvu added the cli/pybindings Command line interface & Python bindings label Oct 11, 2023
@nilsvu
Copy link
Member Author

nilsvu commented Oct 20, 2023

@yoonso0-0 mind taking a quick look?

@nilsvu nilsvu requested a review from knelli2 October 20, 2023 18:37
@nilsvu
Copy link
Member Author

nilsvu commented Oct 31, 2023

@yoonso0-0 and/or @knelli2 please take a look at this

@yoonso0-0
Copy link
Contributor

I'll try to look at this before the end of this week

Comment on lines +1 to +2
{
"cells": [
Copy link
Contributor

Choose a reason for hiding this comment

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

So we already have a "Visualizing volume data" tutorial. This feels like a very natural extension of that tutorial, having two sections

  • Visualizing with paraview GUI
  • Visualizing with spectre python

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a section there with a link to the notebook (can't merge the tutorials because this is a notebook).

Comment on lines +12 to +13
"First, you need access to the `spectre` Python bindings. A simple way to do this\n",
"is to run a Jupyter server from the build directory:\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a link to the python.md tutorial you just edited since that has detailed instructions on how to load the spectre module

Comment on lines +214 to +271
{
"cell_type": "code",
"execution_count": null,
"metadata": {},
"outputs": [],
"source": []
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove?

@nilsvu nilsvu force-pushed the tutorial_vis_python branch 2 times, most recently from 8991033 to 1c6ce22 Compare March 13, 2024 06:41
@nilsvu
Copy link
Member Author

nilsvu commented Mar 13, 2024

Rebased and updated this @knelli2.

@yoonso0-0 mind taking a look as well?

# - Fish:
cp ./bin/python/shell-completion.fish ~/.config/fish/completions/spectre.fish
# - Zsh:
. ./bin/python/shell-completion.zsh
Copy link
Contributor

Choose a reason for hiding this comment

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

So I tried this on develop, but this didn't make any change on my zsh. It actually is effective after sourcing shell-completion.bash.

Copy link
Member Author

@nilsvu nilsvu Mar 29, 2024

Choose a reason for hiding this comment

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

It works for me in ZSH with the instructions here when I have the bin/ directory in the PATH, so I can do just spectre <TAB>. It doesn't work for me when I do ./bin/spectre <TAB>. Seems fine I think. Edit: Actually ./bin/spectre seems to work too.

"from spectre.Visualization.ReadH5 import list_observations\n",
"import glob\n",
"\n",
"h5files = glob.glob(\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add one-line comment like

# Change filename to your sim data

"subfile_name = \"/VolumeData\"\n",
"obs_ids, obs_times = list_observations(open_volfiles(h5files, subfile_name))"
]
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a cell for printing out a list of available variables?

@nilsvu
Copy link
Member Author

nilsvu commented Mar 29, 2024

Updated with your suggestions @yoonso0-0. Thanks for reviewing!

@nilsvu
Copy link
Member Author

nilsvu commented Apr 29, 2024

Please look at again @knelli2. Seems ready to merge.

@knelli2
Copy link
Contributor

knelli2 commented Aug 21, 2024

@nilsvu will update with tutorial from SXS-Con

@nilsvu
Copy link
Member Author

nilsvu commented Aug 21, 2024

@knelli2 you can actually just merge this PR. Then I can update the tutorial with the additions from sxscon in a next PR.

@knelli2
Copy link
Contributor

knelli2 commented Aug 22, 2024

@nilsvu Can you give this a quick rebase? I'll merge after CI passes

@knelli2 knelli2 merged commit 583ab08 into sxs-collaboration:develop Aug 22, 2024
22 checks passed
@nilsvu nilsvu deleted the tutorial_vis_python branch October 22, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli/pybindings Command line interface & Python bindings documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants