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

Update Coupled Model and Delete adf_quick_run.ipynb now that link_to_ADF.ipynb exists #198

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

TeaganKing
Copy link
Collaborator

This PR removes the old adf_quick_run notebook since we now prefer supporting link_to_ADF.ipynb and the helper scripts that create the ADF configuration files

All PRs Checklist:

@mnlevy1981
Copy link
Collaborator

adf_quick_run.ipynb is still run from examples/coupled_model/config.yml:

    atm:
      adf_quick_run:
        parameter_groups:
          none:
            adf_path: ../../../externals/ADF
            config_path: .
            config_fil_str: "config_f.cam6_3_119.FLTHIST_ne30.r328_gamma0.33_soae.001.yaml"

We've talked about encouraging folks to use key_metrics instead of coupled_model, but until we get rid of the latter (which should be done, since it doesn't follow the same variable name conventions as key_metrics and that is confusing) I think we should keep the ADF quick run notebook.

Another concern is that maybe @justin-richling, @brianpm, or @nusbaume want to provide an example of invoking ADF from a notebook instead of the command line. If that's not the case, maybe we could remove the quick run notebook but run a different atmosphere notebook in coupled_model?

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.

Should've marked "request changes" with #198 (comment)

@nusbaume
Copy link
Collaborator

nusbaume commented Mar 6, 2025

Hi @mnlevy1981 @TeaganKing, after discussing it in an ADF meeting we decided that there isn't really a need to have an example of the ADF being called from a notebook (calling it directly in python or from the command line is fine with us), so feel free to move ahead with this particular PR. Thanks for checking!

@TeaganKing
Copy link
Collaborator Author

Thanks @nusbaume for this feedback. @mnlevy1981 I removed this from the coupled model config file, too. That said, I'm not in any rush to get this in, and it could certainly be done as a larger removal (or updating) of the coupled model example.

@TeaganKing TeaganKing changed the title Delete adf_quick_run.ipynb now that link_to_ADF.ipynb exists Update Coupled Model and Delete adf_quick_run.ipynb now that link_to_ADF.ipynb exists Mar 12, 2025
@TeaganKing
Copy link
Collaborator Author

TeaganKing commented Mar 12, 2025

The past few commits address these changes:

  • The register_cmap error is coming from mom6-tools. There are changes in mom6-tools that already address this deprecation, so this PR also now pulls in a more recent mom6-tools external.
  • The robust argument of plot() has also been deprecated and hence removed from the Global_Land_Compare_Two_Cases notebook.
  • Notebook names have been updated to adhere to our suggested format.
  • The 'coupled model' (now 'additional metrics') example configuration file has been updated to be a similar format as the key metrics configuration file.
  • Parameter cells have been updated for the ocean and land notebooks such that the proper parameters are used from the configuration file.

Remaining issues:

  • The land notebook fails on data that does not include 'lon' in line 4: ds[var].sel(lon=300, lat=-10, method="nearest").plot(hue="case", y="levsoi")

@mnlevy1981
Copy link
Collaborator

mnlevy1981 commented Mar 12, 2025

The land notebook fails on data that does not include 'lon' in line 4: ds[var].sel(lon=300, lat=-10, method="nearest").plot(hue="case", y="levsoi")

Is this a case that was run on the CAM-SE grid instead of the FV grid? I bet something like

if "lndgrid" in ds[var].dims:
    # find index of lndgrid close to 300 E, 10 S
    ds[var].isel(lndgrid=lndgrid_ind).plot(hue="case", y="levsoi")
else:
    ds[var].sel(lon=300, lat=-10, method="nearest").plot(hue="case", y="levsoi")

would work (where the tricky part is finding the lndgrid index closest to (300,-10): I would do it offline once, and then hard-code the index with a comment about where it came from and how it's only useful for the ne30 grid); alternatively, we could require this notebook to use regridded output if it was run on the SE grid.

@TeaganKing
Copy link
Collaborator Author

I found the closest land indices by finding a minimum non-NaN distance between the landgrid and the lat/lon values.

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