Intake conversion Decomposing_kinetic_energy_into_mean_and_transient

[rbeucher]

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.

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[review-notebook-app[bot]]

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

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Delete?

review-notebook-app[bot] commented 2 months ago

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."

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Delete

review-notebook-app[bot] commented 2 months ago

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'] ?

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

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

Delete?

review-notebook-app[bot] commented 2 months ago

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."

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Delete

review-notebook-app[bot] commented 2 months ago

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'] ?

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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."

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Delete

review-notebook-app[bot] commented 2 months ago

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'] ?

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

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

Delete

review-notebook-app[bot] commented 2 months ago

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'] ?

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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'] ?

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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

review-notebook-app[bot] commented 2 months ago

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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.

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

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

This hasn't worked either?

review-notebook-app[bot] commented 2 months ago

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

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

Fix

review-notebook-app[bot] commented 2 months ago

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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

review-notebook-app[bot] commented 2 months ago

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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?

review-notebook-app[bot] commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

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 commented 2 months ago

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 commented 2 months ago

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.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[julia-neme]

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]

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.