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

Adds an updated version of the Jupyter notebook tutorial for the onse… #5533

Merged
merged 7 commits into from
Mar 28, 2024

Conversation

ljhwang
Copy link
Contributor

@ljhwang ljhwang commented Dec 19, 2023

Back at the 2017 hack in Georgia, the original onset of convection tutorials were converted to Jupyter notebooks.

This adds an updated version of the notebook to the cookbook directory that the tutorial is related to.

Updates include:

  • modifications to the prm files. Note that between versions the structure changed.
  • modifications to the Nusselt-Rayleigh number relationship exercise. Note with the substantial improvements to the solver, the results changed. I have left the previous numbers in for historic reasons and arbitrarily reset the end times. Feel free to suggest a different set of parameters.
  • corrected a bug in plotting the Nusselt-Rayleigh number plot. Verification by the reviewer that this is now correct is warranted.

The updated version is designed to work using the ASPECT Jupyterlab environment on geodynamics.org: https://geodynamics.org/resources/aspectnotebook

The instructions to run in the old Docker container and the old parameter files are retained. This could be removed if a new docker container is made or this is confusing.

@tjhei
Copy link
Member

tjhei commented Dec 27, 2023

Can you please remove all the checkpoint files you accidentally added?

@ljhwang
Copy link
Contributor Author

ljhwang commented Dec 27, 2023

@tjhei Thanks for the catch on the hidden files. Hopefully this is clean now.

@bangerth
Copy link
Contributor

bangerth commented Jan 9, 2024

You'll have to merge the two commits (squash or fix-up) to ensure these large files are not in one commit and then removed in the next, leaving them as large (but unnecessary) additions to the repository.


## Running on your desktop

If you run in your local JupyetrLab environment, note the following dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If you run in your local JupyetrLab environment, note the following dependencies:
If you run in your local JupyterLab environment, note the following dependencies:

@@ -0,0 +1,161 @@
# A description of convection in a 2d box. See the manual for more information.
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why leave old stuff around? Documentation should document the current state of a code base; it is not helpful to anyone to document a previous version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always a good question. The reason to keep this around is that there is not a new docker container with the most recent code.

@ljhwang ljhwang force-pushed the tutorial-convection branch from 16c4239 to d77075c Compare January 19, 2024 17:26
@ljhwang
Copy link
Contributor Author

ljhwang commented Jan 19, 2024

@bangerth. Both of your comments have been addressed and this should be ready to be merged. Once merged, I will remove the old prm files and update the README to be consistent. ty.

Copy link
Member

@tjhei tjhei left a comment

Choose a reason for hiding this comment

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

Hi Lorraine, thank you for putting this together. This is great to have as a resource. I think we need to discuss a couple of things first, though.


The current version is verified to run within the ASPECT Jupyter Notebooks tool which can be launched from the CIG website:

https://geodynamics.org/resources/aspectnotebook
Copy link
Member

Choose a reason for hiding this comment

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

This is not documented here or on the page linked here: It seems that you have to add $ASPECT_DIR to the path manually before you can start "aspect". @gassmoeller is that true or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is in the default path for the notebook container.

Copy link
Member

Choose a reason for hiding this comment

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

@tjhei: Lorraine is correct, $ASPECT_DIR is already set inside the tool. No need for the user to set it.

Copy link
Member

Choose a reason for hiding this comment

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

If I click on https://geodynamics.org/resources/aspectnotebook, then 'launch tool', "new terminal" I get:

timoheister@geodynamics_1099_6:~$ aspect
bash: aspect: command not found

while $ASPECT_DIR/bin/aspect works.

A. Install Docker

B. Download the docker image
> docker pull tjhei/aspect-jupyter
Copy link
Member

Choose a reason for hiding this comment

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

Should we maybe update this image first instead of trying to be compatible with it? This image is from 2017 and contains ASPECT 2.0.0-pre. If we don't update it, we should remove these instructions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The plan is to remove these files and clean-up the README.md file after this pull request is merged. This way we have a record of it somewhere in the commit history if needed.

# A description of convection in a 2d box. See the manual for more information.


# At the top, we define the number of space dimensions we would like to
Copy link
Member

Choose a reason for hiding this comment

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

This and many other files are in a folder .ipynb_checkpoints/. I think this temporary folder should not be part of your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh drats, you did find more. Do you mind if i clean this up when i get rid of the "old" files?

@@ -0,0 +1,111 @@
{
"cells": [
Copy link
Member

Choose a reason for hiding this comment

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

why is this file not in the notebooks folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. The philosophy here is to put the "obvious" starting point at the top level. Agnostic about this. what do others think?


# Introduction to ASPECT

This notebook is based on tutorials by J. Dannberg that provide a basic introduction to ASPECT.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a better place to put notebooks/tutorials? It seems weird to me to hide them inside the convection-box folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was part of a broader discussion to keep notebooks as close as possible to the related prm's such that they stay current.

@gassmoeller
Copy link
Member

I removed the checkpoint files.

@bangerth
Copy link
Contributor

bangerth commented Mar 9, 2024

It fails the indentation check with these diffs:

diff --git a/cookbooks/convection-box/tutorial-onset-of-convection/README.md b/cookbooks/convection-box/tutorial-onset-of-convection/README.md
index a1d0535..1668690 100644
--- a/cookbooks/convection-box/tutorial-onset-of-convection/README.md
+++ b/cookbooks/convection-box/tutorial-onset-of-convection/README.md
@@ -29,11 +29,11 @@ If you run in your local JupyterLab environment, note the following dependencies
  * ipympl
  * scipy
  * glob
- 
- 
- 
+
+
+
 ## Packages install
- 
+
 The heat flux slider requires the installation of two additional packages.
 
 **Adding Jupyter widgets**
@@ -43,5 +43,3 @@ You should have added this in Step 3 above:
 
 **Installing tables**
 > conda install tables
- 
- 
\ No newline at end of file
diff --git a/cookbooks/convection-box/tutorial-onset-of-convection/model_input/tutorial.prm b/cookbooks/convection-box/tutorial-onset-of-convection/model_input/tutorial.prm
index d6f0ff2..3686151 100644
--- a/cookbooks/convection-box/tutorial-onset-of-convection/model_input/tutorial.prm
+++ b/cookbooks/convection-box/tutorial-onset-of-convection/model_input/tutorial.prm
@@ -[14](https://github.com/geodynamics/aspect/actions/runs/7674802326/job/20920049230?pr=5533#step:6:15)7,4 +147,3 @@ subsection Postprocess
     set Data output format = vtu
   end
 end
-

@bangerth
Copy link
Contributor

bangerth commented Mar 9, 2024

The other failures seem to come from the fact that the root of the branch is at a point in time where we had the GMG failures.

@gassmoeller gassmoeller force-pushed the tutorial-convection branch from a84e412 to 3f0edc7 Compare March 15, 2024 17:14
@gassmoeller gassmoeller force-pushed the tutorial-convection branch from f2d0c13 to 88b3995 Compare March 15, 2024 18:00
@gassmoeller
Copy link
Member

I indented, rebased and fixed a number of remaining typos, this should be good to go @ljhwang.

@gassmoeller gassmoeller merged commit 6d2e1af into geodynamics:main Mar 28, 2024
8 checks passed
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.

4 participants