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

Demo notebook for updated metrics functions #18

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jdldeauna
Copy link

@jdldeauna jdldeauna commented Jul 16, 2021

Demonstrating updated functionality for metrics functions in xgcm

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -0,0 +1,2393 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you comment the first line out? Just to see if that is the culprit for the failures we are seeing?


Reply via ReviewNB

Copy link
Author

@jdldeauna jdldeauna Jul 21, 2021

Choose a reason for hiding this comment

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

Looking at the Binderbot check results, the error comes from 01_ecco and transform_mld_coordinates. The 06_metrics notebook renders fine.

@@ -0,0 +1,2393 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you get a cluster via dask gateway. That might also contribute to some errors.


Reply via ReviewNB

@jbusecke
Copy link
Collaborator

I am not sure what is going on. My best guess for now. The first notebook failure is some temporary glitch (https error) and the mld notebook failure might be due to the install command you still had in the notebook? If fixing that doesnt help, we will need to dig deeper into what has changed on the docker image recentyl

@jdldeauna
Copy link
Author

Hmm I'm not sure if this affects anything, but to update the notebook I run it in Pangeo Cloud, then download it to my computer so I can then push changes to this repository. Is there a way to push directly from Pangeo Cloud?

@jbusecke
Copy link
Collaborator

Oh yeah for sure. You can open a terminal on there:
image
Just click on the big blue + button up top and choose Terminal on the lower left.

@dcherian
Copy link

dcherian commented Jul 22, 2021

This is looking great. I really liked how you show the big result up front (grid.average(uo) works) and dive in to all the effort it took to get it to work.

Some higher level comments to make it even more awesome.:

  1. I think you should show 1 or maybe 2 years of data so people can see the seasonal pattern. Right now it looks very noisy.
  2. I think you need a simple 4-cell version of this figure with T-points and u-points to make it really clear to the audience what needs to happen. you could illustrate what "averaging" or "interp"-ing does using a version of the 4-cell image too
    image
  3. The "setup" (reading, subsetting, plotting) is a little long. While presenting I think you may want to do this quickly mostly highlighting "this is a regional subset of the global model and I want to analyze it".

Copy link
Author

Great! :)

  1. Alright, will do.
  2. Do you mean a graphic of 4 boxes with the dot in the middle for temperature, and arrows at the sides for u-velocity? Will this be embedded in the notebook as well? Maybe before the cell where I calculate mean SST time series?
  3. For sure, I will just go through that quickly. The main focus will be showing how calculating u-velocity average took longer code before, how much shorter it is now, and the part at the end where it shows how this is made possible.

@dcherian
Copy link

Do you mean a graphic of 4 boxes with the dot in the middle for temperature, and arrows at the sides for u-velocity? Will this be embedded in the notebook as well?

Yes. You can highlight a "u cell" and a "T cell" and show that areas are potentially different.

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