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

Intake conversion Decomposing_kinetic_energy_into_mean_and_transient #358

Merged

Conversation

rbeucher
Copy link
Contributor

@rbeucher rbeucher commented Jun 6, 2024

Following the discussion in issue #313, we propose converting the recipes to use Intake, given that the Cookbook is no longer supported and the ACCESS-NRI Intake catalog is now available.

A few months ago, @max-anu began working on this transition. This pull request contains the changes @max-anu made to the notebook specified in the title.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:29Z
----------------------------------------------------------------

Line #8.    from cosima_cookbook import distributed as ccd

Can this be replaced by .load() or .compute()? Otherwise this notebook still uses the soon-to-be-gone cookbook


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:29Z
----------------------------------------------------------------

Delete?


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:30Z
----------------------------------------------------------------

Rephrase: "Choose an experiment which has daily velocities saved for the Southern Ocean."


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:31Z
----------------------------------------------------------------

Delete


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:31Z
----------------------------------------------------------------

Line #5.    u = darray

Simplify to u = darray['u'] ?


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:32Z
----------------------------------------------------------------

As I said above, I think when moving to intake we should stop using the cosima_cookbook


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:33Z
----------------------------------------------------------------

This doesn't seem right - should look like the plot on the left.


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:33Z
----------------------------------------------------------------

I don't see where KE_dz was defined?


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:34Z
----------------------------------------------------------------

This hasn't worked either?


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:35Z
----------------------------------------------------------------

Fix


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:35Z
----------------------------------------------------------------

Why not start with this functions already? Seems to me a bit repetitive


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:36Z
----------------------------------------------------------------

Line #1.    @memory.cache

This is probably my ignorance, but what does this line do?


Copy link

review-notebook-app bot commented Jul 7, 2024

View / edit / reply to this conversation on ReviewNB

julia-neme commented on 2024-07-07T23:53:37Z
----------------------------------------------------------------

Line #5.        ncfiles=[str(f[0].ncfile_path)  for f in cc.querying._ncfiles_for_variable(expt, 'u', session) if 'ocean_daily_3d' in str(f[0].ncfile_path) ]

This is still using the cookbook


@julia-neme
Copy link
Collaborator

I'm sorry I don't know why all the comments where posted here. I think there are a couple of problems with this notebook:

  • It is still using the cookbook to load variables.
  • It is still using the cookbook to use .compute_by_blocks(). Isn't there a non-cookbook alternative?
  • I think there is a bit of duplication, doing the exact same thing using and not using functions. I think this could be condensed by using well commented functions.
  • Is it worth is trying to make this model agnostic too?

@adele-morrison
Copy link
Collaborator

Thanks for looking at this @julia-neme. I’m not sure if you realised, but @rbeucher and @max-anu who submitted these aren’t going to work on them anymore. Would you have time to implement these changes you suggest, then someone else can give it a second review? Feel free to start again from the most recent version of the notebook to convert to the intake catalog (and close this notebook) and do it yourself from scratch if that’s cleaner / easier. Or work from this one, up to you.

@navidcy navidcy merged commit 9c80e9f into main Aug 27, 2024
3 checks passed
@navidcy navidcy deleted the INTAKE_Decomposing_kinetic_energy_into_mean_and_transient branch August 27, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants