Skip to content

Fix legacy code, typos, suppress run_time warnings, added further support for PyVista visualisations, flagged all kernel killing code raised in Issue #46 #47

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

harley-kelly
Copy link

Fix legacy code for loading data with iris. Corrected any and all typos. Added lines to suppress run_time warnings which bulk up output. Added further support for PyVista visualisations using trame. Flagged all kernel killing code raised in Issue #46 in bold red font.

code in the following notebooks:
- notebooks/Exercise_01.ipynb
- notebooks/Sec_01_Load_and_Examine.ipynb
- notebooks/Sec_03_Plotting.ipynb
- notebooks/Sec_04_Regridding.ipynb
- notebooks/Sec_05_RegionExtraction.ipynb
- notebooks/testdata_fetching.py
…window when using VSCode, flagged where kernel crashes take place.
@CLAassistant
Copy link

CLAassistant commented Apr 17, 2025

CLA assistant check
All committers have signed the CLA.

@bjlittle bjlittle requested a review from ESadek-MO April 23, 2025 09:39
@ESadek-MO ESadek-MO self-assigned this Apr 24, 2025
Copy link

@ESadek-MO ESadek-MO left a comment

Choose a reason for hiding this comment

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

Thank you so much @harley-kelly! It's great for this repo to get some TLC.

The majority of these comments are to clean up any fingerprints. Once those are removed, I should probably have another look, as the outputs can make it hard to see the trees for the wood!

If there's any comments you disagree with, please do so, there's a chance I've lost some context due to the outputs etc.

@@ -83,7 +82,7 @@
},
{
"cell_type": "code",
"execution_count": null,
"execution_count": 8,
"id": "b8eb1ce2-2160-4c3f-b97b-4b09946d84cc",
"metadata": {
"tags": []

Choose a reason for hiding this comment

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

Github won't allow me to comment on relevant lines, but below, steps 3 and 4 are labelled the wrong way round, seems within scope of this PR.

Choose a reason for hiding this comment

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

GitHub won't allow me to see this in the browser; I'll review this file later on more manually.

In the meantime, please could you double check that there's no outputs/execution numbers included?

Copy link

@ESadek-MO ESadek-MO left a comment

Choose a reason for hiding this comment

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

Hi @harley-kelly,

Sorry for the delay on getting back to this, we've been a bit busy with sprinting.

There're still a few fingerprints here, but for the most part it seems good.

We are really grateful that you've made this contribution, and I'm concious you might not have time to keep referring back! If you do, great, that would be ideal, but if you don't and you'd like us to take over the pull request (with credit to you, of course), we'd be happy to!

Comment on lines +1703 to +1704
"iris.FUTURE.date_microseconds = True\n",
"\n",

Choose a reason for hiding this comment

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

This is still needed!

@@ -179,8 +175,35 @@
"outputs": [],
"source": [
"um_cube = iris.load_cube(um_filepth,'air_temperature')\n",
"#print(um_cube)\n",
"print(um_cube.mesh)\n"
"print('You can see the data here: \\n \\n', um_cube)"

Choose a reason for hiding this comment

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

Suggested change
"print('You can see the data here: \\n \\n', um_cube)"
"print('You can see the data here: \n \n', um_cube)"

@@ -195,7 +218,7 @@
],
"metadata": {
"kernelspec": {
"display_name": "Python 3 (ipykernel)",
"display_name": "everyday",

Choose a reason for hiding this comment

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

Suggested change
"display_name": "everyday",
"display_name": "Python 3 (ipykernel)",

@@ -12,14 +12,18 @@
},
{
"cell_type": "code",
"execution_count": null,
"execution_count": 68,

Choose a reason for hiding this comment

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

Suggested change
"execution_count": 68,
"execution_count": null,

@@ -135,7 +139,7 @@
},
{
"cell_type": "code",
"execution_count": null,
"execution_count": 74,

Choose a reason for hiding this comment

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

Suggested change
"execution_count": 74,
"execution_count": null,

Comment on lines +503 to +516
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"[[792.0181318972558]\n",
" [26.95742764760166]\n",
" [146.36362300827432]\n",
" [166.02688348059408]\n",
" [427.4766774994878]\n",
" [263.7582442600861]]\n"
]
}
],

Choose a reason for hiding this comment

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

Suggested change
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"[[792.0181318972558]\n",
" [26.95742764760166]\n",
" [146.36362300827432]\n",
" [166.02688348059408]\n",
" [427.4766774994878]\n",
" [263.7582442600861]]\n"
]
}
],
"outputs": [],

Choose a reason for hiding this comment

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

This file needs the Jupyter fingerprint removed as well!

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.

3 participants