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

Compute land-atmosphere terrestrial coupling index in key_metrics #145

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

megandevlan
Copy link
Contributor

@megandevlan megandevlan commented Oct 16, 2024

All Submissions:

  • [ y] Have you followed the guidelines in our Contributor's Guide (including the pre-commit check)?
  • [ y] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Does your submission pass tests? -- Not sure
  2. Have you lint your code locally prior to submission? -- Not sure

Changes to Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them? -- In conversation, yes.
  • [y ] Have you successfully tested your changes locally?

@mnlevy1981
Copy link
Collaborator

Thanks for opening this PR!

A few comments before diving into the nuts and bolts:

  1. Can you please run pre-commit (and set it up to run on all future commits from your CUPiD directory)? Instructions are in the wiki (specifically, in the Contributor's Guide). I would recommend running both pre-commit run --all-files and pre-commit install from (cupid-dev), and then committing additional changes to the branch from that environment as well
  2. There are a lot of empty cells in the notebook that can probably be removed. Also, adding the hide-input tag will hide the input cell when creating the JupyterBook webpage while hide-cell will hide both the input and output; you might want to add one of those or the other to each cell in your notebook to make the webpage a little easier to view
  3. Can you please remove all the output from the notebook in examples/nblibrary/? Just a simple "restart kernel and clear output" before saving will keep it as a blank template.
  4. I see a bunch of /glade/derecho/scratch/mdfowler/tmp/ipykernel_13813/1925081246.py:42: DeprecationWarning: Conversion of an array with ndim > 0 to a scalar is deprecated, and will error in future. Ensure you extract a single element from your array before performing this operation. (Deprecated NumPy 1.25.), and I think the warnings are pointing to lat_fluxnet[iStation] = siteInfoDF['LOCATION_LAT'].values[indStation] and lon_fluxnet[iStation] = siteInfoDF['LOCATION_LONG'].values[indStation] in [9]; can you figure out the correct indexing to make sure both sides of the equation are scalars and avoid this warning? (I'm happy to help you play with that if you'd like, though I won't have a chance until tomorrow)

Once those bigger updates are made, I'll look more closely at the notebook

@megandevlan
Copy link
Contributor Author

I've made most of those changes. For some reason, the tags for hide-input and hide-output aren't working as expected, but otherwise I think it's better!

Copy link
Collaborator

@mnlevy1981 mnlevy1981 left a comment

Choose a reason for hiding this comment

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

When I run this, I see the warnings from LandAtm_CouplingIndex_V2.ipynb:

----------- /glade/work/mlevy/codes/CUPiD/examples/nblibrary/lnd/LandAtm_CouplingIndex_V2.ipynb ------------
:5:1: 'datetime' imported but unused
:6:1: 'datetime.date' imported but unused
:7:1: 'dask' imported but unused
:14:1: 'cartopy' imported but unused
========================================

Could you remove these imports? Also, it looks like Global_TerrestrialCouplingIndex_VisualCompareObs.ipynb isn't ever run -- could you either add it to config.yml or remove it from the repo?

Lastly, a couple of things from cupid-build:

  1. the config.yml isn't set up to generate the webpage... could you make the following changes?

    --- a/examples/key_metrics/config.yml
    +++ b/examples/key_metrics/config.yml
    @@ -194,9 +194,9 @@ book_toc:
         #   chapters:
         #       - file: ocn/ocean_surface
    
    -    # - caption: Land
    -    #   chapters:
    -    #     - file: lnd/land_comparison
    +    - caption: Land
    +      chapters:
    +        - file: lnd/LandAtm_CouplingIndex_V2
    
         # - caption: Sea Ice
         #   chapters:
  2. I get the following warnings from cupid-build:

    /glade/work/mlevy/codes/CUPiD/examples/key_metrics/computed_notebooks/key_metrics/lnd/LandAtm_CouplingIndex_V2.ipynb:20002: WARNING: skipping unknown output mime type: application/vnd.holoviews_load.v0+json [mystnb.unknown_mime_type]
    /glade/work/mlevy/codes/CUPiD/examples/key_metrics/computed_notebooks/key_metrics/lnd/LandAtm_CouplingIndex_V2.ipynb:20002: WARNING: skipping unknown output mime type: application/vnd.holoviews_load.v0+json [mystnb.unknown_mime_type]
    /glade/work/mlevy/codes/CUPiD/examples/key_metrics/computed_notebooks/key_metrics/lnd/LandAtm_CouplingIndex_V2.ipynb:20002: WARNING: skipping unknown output mime type: application/vnd.holoviews_exec.v0+json [mystnb.unknown_mime_type]
    

    Could you verify that the website contains all the output you expect? It might be the case that some plots aren't being parsed right by the jupyter book step. I think everything from the notebook is included -- it would be nice to figure out how to avoid those messages, but that could be handled in a separate PR if the website itself looks okay

@megandevlan
Copy link
Contributor Author

I'm not sure where the error messages are coming from, but I can confirm that the website looks good now! And things are updated to point to the right file name.

@mnlevy1981
Copy link
Collaborator

This looks great! One more question before I merge it -- it looks like you added examples/nblibrary/lnd/image.png to the repository. Is that on purpose? I don't think the image is used anywhere.

@megandevlan
Copy link
Contributor Author

Nope I didn’t add that on purpose, but I think it auto-generated when I ran the script. I’ll delete it and re-commit :)

@mnlevy1981 mnlevy1981 merged commit 2dda82f into NCAR:main Nov 18, 2024
1 of 2 checks passed
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.

2 participants