Created neutral density example notebook #417

julia-neme · 2024-07-08T09:47:28Z

This notebook still uses the cookbook, and could be parallelised for efficiency (but I don't know how to do the latter).

review-notebook-app · 2024-07-08T09:47:33Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

navidcy · 2024-07-09T05:04:21Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Please add in the method's doc that it also slices the data to a particular region?

Reply via ReviewNB

navidcy · 2024-07-09T05:04:21Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Could we then have another example after the plots to do the same with 'mom6' output?

Reply via ReviewNB

navidcy · 2024-07-09T05:04:21Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Line #10. gamma[time_step,:,:,longitude] = gamma_XT
Spaces after commas please :)

Reply via ReviewNB

navidcy · 2024-07-09T05:04:21Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Line #11. # Print the time_step so the impatients know how much is left to do
But how do the impatients know how how many time steps are there? perhaps print(100*time_step/T, "% done") or something?

Reply via ReviewNB

navidcy · 2024-07-09T05:04:21Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Instead of python comments, could we have markdown explanations before the code cell?

(That's a general comment, not only for this cell!)

Reply via ReviewNB

navidcy

Thanks for this @julia-neme! I added few comments!

julia-neme · 2024-07-10T01:53:18Z

Done this fixes, but might wait until I get access to xp65 to convert to intake, and maybe till pygamma gets in the conda environments :) Thanks Navid

navidcy · 2024-07-10T04:16:45Z

Done this fixes, but might wait until I get access to **xp65** to convert to intake, and maybe till pygamma gets in the conda environments :) Thanks Navid

We'll put pygamma_n into the conda environment -- that should be easy!

navidcy · 2024-07-10T04:17:36Z

~~Can you link this PR with the issue by editing the first comment in the PR and write Closes #XYZ where XYZ is the issue number?~~

The issue is linked -- never mind!

navidcy · 2024-07-10T12:30:34Z

Examples/Neutral_density_calculation.ipynb

@@ -0,0 +1,790 @@
+{


Line #1. t, s = load_temp_salt('mom5')
Can we use T, S for temperature and salinity? t is often used for time....

Reply via ReviewNB

… to install pygamma since it will be incorporated to conda environments

julia-neme · 2024-07-16T06:41:13Z

I don't know how/why I've closed this pull request. To finish the notebook I need to choose between:

Using the cookbook instead of intake to do it model agnostic
Using intake but working only with ACCESS-OM2
@navidcy what do you think is best? I can also wait until either those are fixed by (NCI?)

anton-seaice · 2024-07-16T23:49:05Z

I would stick with the cookbook until we have a MOM6 run in the catalogue (i.e. ACCESS-NRI/access-nri-intake-catalog#175)

anton-seaice · 2024-07-16T23:54:38Z

It looks like you deleted/renamed the "master" branch in your fork @julia-neme ? I guess you were doing this to try and get your fork up to date.

Because the PR is trying to merge into COSIMA:main from julia-neme:master this has had the side-effect of closing the pull request (I assume) because julia-neme:master doesn't exist anymore.

You can either make a new branch in your fork called 'master' (using commit 0193c90) and then we can reopen this PR. Or make a new branch from that commit but that would need a new PR.

julia-neme · 2024-07-17T00:35:23Z

Thanks a lot Anton!!

Examples/Neutral_density_calculation.ipynb

navidcy

added couple of comments via ReviewNB

navidcy · 2024-07-26T01:00:01Z

@julia-neme this notebook needs ~1hr to run????

navidcy · 2024-07-26T01:02:53Z

Could we make it shorter? It's only an example -- doesn't need to be publication ready...

navidcy

Looks good to me (but I haven't approved yet because I need to run it and it takes forever!!!!)

Could we make this notebook run faster, e.g., by loading less data?

There are two nested-for loops which are a huuuuuuge hit to performance and not xarray-friendly. Is this a restriction of pygamma_n? Perhaps xarray.apply_ufunc could come to the rescue here??

navidcy · 2024-07-26T03:01:01Z

@jemmajeffree is this an case where xarray.apply_ufunc could be really useful?

I'm referring to the cells that do

for time_step in range(0, Nt):
    for longitude in range(0, Nx):
        do_stuff()

that make this notebook really really slow...

jemmajeffree · 2024-07-26T04:26:15Z

@jemmajeffree is this an case where xarray.apply_ufunc could be really useful?

Yes! absolutely, it looks like a perfect example. I'll try and get to it in the next week

Can anyone at ANU walk me though how to make changes within the pull request? (this is the main thing I've been avoiding with regards to fixing up the apply_ufuncs example, sorry)

Something to note: I can probably speed it up (don't know by how much until I poke it), but it will come at a cost of less intuitive and readable code. How much do we/this repo value each of these two priorities?

julia-neme · 2024-07-26T04:41:05Z

You can look how to review here

After making the changes I think you can push back your modifications into here. I'm having a go at parallelising this. It's running in 1min now, so I guess that's good enough?

Parallelising is also very readable, just one line of code.

navidcy · 2024-07-26T05:14:40Z

@jemmajeffree just make changes and commit to that branch and push and your PR will be updated

I can help you via zoom despite you requested for ANU-onsite help above ;)

navidcy · 2024-07-26T05:15:23Z

After making the changes I think you can push back your modifications into here. I'm having a go at parallelising this. It's running in 1min now, so I guess that's good enough?

Parallelising is also very readable, just one line of code.

1min????
before it was 1hr....! YES that's very OK!!! That's great!

julia-neme · 2024-07-26T05:17:02Z

Yep! Do we still want the ufuncs or the parallelisation?

navidcy · 2024-07-26T05:25:19Z

I'm happy to approve something that runs in 1min... But I haven't seen anything pushed... :)

julia-neme · 2024-07-26T05:26:15Z

I don't know how to integrate the "tweaks" you've pushed jaja. Trying to figure that out. Without loosing my updates

navidcy · 2024-07-26T05:29:03Z

I can help you with that ;)

navidcy · 2024-07-26T05:41:59Z

FYI: I did made a few changes in the model_args so that now less if model=='momX' statements are needed..

navidcy · 2024-07-26T05:46:50Z

Something to note: I can probably speed it up (don't know by how much until I poke it), but it will come at a cost of less intuitive and readable code. How much do we/this repo value each of these two priorities?

It's a balance...

I'd say a 1.5x-2x speedup in the cost of less intuitive-readable code is not really worth it. But a 30x or 100x speedup it worths it!

jemmajeffree · 2024-07-26T05:51:27Z

Yep! Do we still want the ufuncs or the parallelisation?

If you can parallelise without ufuncs do that. apply_ufuncs is a way of parallelising functions that refuse to do so natively, but you're better off with parallelising within a function if it supports it

navidcy

this is great!

navidcy · 2024-07-26T05:59:01Z

I think it's cleaner done with ufunc but this is now great! The whole notebook runs in 4min instead of 2hrs so THAT'S GREAT.

I suggest opening an issue and pointing to this notebook saying that parallelization could be done via xarray.apply_ufunc and this is something somebody could tackle in the future ;)

navidcy · 2024-07-26T05:59:25Z

@julia-neme you may merge this whenever you feel like it!

julia-neme and others added 3 commits July 8, 2024 19:40

Created neutral density example

62f3150

Merge branch 'COSIMA:main' into master

1d822bc

Moved Neutral_density_calculation.ipynb to Examples

5083c0d

julia-neme linked an issue Jul 8, 2024 that may be closed by this pull request

Neutral density calculation example #387

Closed

julia-neme self-assigned this Jul 8, 2024

navidcy added the 🐣 good first issue Good for newcomers label Jul 9, 2024

navidcy reviewed Jul 9, 2024

View reviewed changes

navidcy requested changes Jul 9, 2024

View reviewed changes

navidcy added 🐣 new example 🕹️ hackathon 4.0 and removed 🐣 good first issue Good for newcomers labels Jul 9, 2024

Added mom6

d0acdd6

navidcy reviewed Jul 10, 2024

View reviewed changes

julia-neme added 2 commits July 16, 2024 15:50

Fixed reviewer's comments, changed to intake and removed instructions…

e98b46f

… to install pygamma since it will be incorporated to conda environments

Merge remote-tracking branch 'upstream/main'

0193c90

julia-neme force-pushed the master branch from 9c53b14 to 0193c90 Compare July 16, 2024 05:53

julia-neme closed this Jul 16, 2024

julia-neme deleted the master branch July 16, 2024 06:03

julia-neme restored the master branch July 17, 2024 00:32

julia-neme reopened this Jul 17, 2024

julia-neme added 2 commits July 22, 2024 11:15

Merge branch 'main' into master

4a70f19

Have commented out the mom6 part because it is not indexed yet in intake

543caff

navidcy reviewed Jul 23, 2024

View reviewed changes

Examples/Neutral_density_calculation.ipynb Outdated Show resolved Hide resolved

Examples/Neutral_density_calculation.ipynb Outdated Show resolved Hide resolved

navidcy reviewed Jul 23, 2024

View reviewed changes

Merge branch 'main' into master

b28df66

navidcy self-requested a review July 26, 2024 00:36

navidcy added 2 commits July 26, 2024 10:56

some code cleanup

aa8bca1

rename

7e54a9c

navidcy reviewed Jul 26, 2024

View reviewed changes

some tweaks

003a8bb

parallelised

691ed7f

parallelised neutral density

4c45caa

julia-neme and others added 2 commits July 26, 2024 15:51

Delete Examples/Neutral_density_calculation.ipynb

aebf8f0

remove the comment about future potential parallelisation

7fe9d76

navidcy approved these changes Jul 26, 2024

View reviewed changes

julia-neme merged commit 1b7a689 into COSIMA:main Jul 26, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Created neutral density example notebook #417

Created neutral density example notebook #417

julia-neme commented Jul 8, 2024

review-notebook-app bot commented Jul 8, 2024

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy left a comment

julia-neme commented Jul 10, 2024

navidcy commented Jul 10, 2024

navidcy commented Jul 10, 2024 •

edited

Loading

navidcy Jul 10, 2024 •

edited

Loading

julia-neme commented Jul 16, 2024

anton-seaice commented Jul 16, 2024

anton-seaice commented Jul 16, 2024

julia-neme commented Jul 17, 2024

navidcy left a comment

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy left a comment

navidcy commented Jul 26, 2024 •

edited

Loading

jemmajeffree commented Jul 26, 2024

julia-neme commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

julia-neme commented Jul 26, 2024

navidcy commented Jul 26, 2024

julia-neme commented Jul 26, 2024 •

edited

Loading

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

jemmajeffree commented Jul 26, 2024

navidcy left a comment

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

Created neutral density example notebook #417

Created neutral density example notebook #417

Conversation

julia-neme commented Jul 8, 2024

review-notebook-app bot commented Jul 8, 2024

navidcy Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

navidcy Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

navidcy Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

navidcy Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

navidcy Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

navidcy left a comment

Choose a reason for hiding this comment

julia-neme commented Jul 10, 2024

navidcy commented Jul 10, 2024

navidcy commented Jul 10, 2024 • edited Loading

navidcy Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

julia-neme commented Jul 16, 2024

anton-seaice commented Jul 16, 2024

anton-seaice commented Jul 16, 2024

julia-neme commented Jul 17, 2024

navidcy left a comment

Choose a reason for hiding this comment

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy left a comment

Choose a reason for hiding this comment

navidcy commented Jul 26, 2024 • edited Loading

jemmajeffree commented Jul 26, 2024

julia-neme commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

julia-neme commented Jul 26, 2024

navidcy commented Jul 26, 2024

julia-neme commented Jul 26, 2024 • edited Loading

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

jemmajeffree commented Jul 26, 2024

navidcy left a comment

Choose a reason for hiding this comment

navidcy commented Jul 26, 2024

navidcy commented Jul 26, 2024

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy Jul 9, 2024 •

edited

Loading

navidcy commented Jul 10, 2024 •

edited

Loading

navidcy Jul 10, 2024 •

edited

Loading

navidcy commented Jul 26, 2024 •

edited

Loading

julia-neme commented Jul 26, 2024 •

edited

Loading