Skip to content

Finite-amplitude Rossby Wave Diagnostics#566

Open
csyhuang wants to merge 192 commits intoNOAA-GFDL:mainfrom
csyhuang:finite_amplitude_wave_diag_fcca20a
Open

Finite-amplitude Rossby Wave Diagnostics#566
csyhuang wants to merge 192 commits intoNOAA-GFDL:mainfrom
csyhuang:finite_amplitude_wave_diag_fcca20a

Conversation

@csyhuang
Copy link
Copy Markdown

@csyhuang csyhuang commented May 20, 2024

Description

This is preliminary PR for an additional POD, "Finite-amplitude Rossby Wave Diagnostics". The code could run through and produce the plots on HTML properly when lastly touched base with Jess #514 (latest commit from main in this PR: f69e745 ) , but it breaks after merging the recent commits. After discussing with my liason @yihungkuo , it seems easier on both sides for me to submit a preliminary PR to iterate (rather than fixing the issues on my own). I am therefore submitting this PR to kickstart the fix, with the understanding that the MDTF team is busy and will get back only when time allows.

Please let me know if I shall provide more information to fix this. On my side, the missing items include the (1) Detailed documentation and (2) predigest data, which do not affect the test of the code. I am still working with my collaborators to finalize them, hoping to get them ready by the end of this month.

Thanks in advance! 😄

How Has This Been Tested?

In my inputdata/ directory, I have the following files downloaded from GFDL portal:

  • model/GFDL-CM3_historical_r1i1p1/day/GFDL-CM3_historical_r1i1p1.ua.day.nc
  • model/GFDL-CM3_historical_r1i1p1/day/GFDL-CM3_historical_r1i1p1.va.day.nc
  • model/GFDL-CM3_historical_r1i1p1/day/GFDL-CM3_historical_r1i1p1.ta.day.nc

I run MDTF with the following command and get all the plots successfully:

./mdtf -f src/default_finite_amplitude_wave_diag.jsonc -v

Checklist:

  • My branch is up-to-date with the NOAA-GFDL main branch, and all merge conflicts are resolved
  • The scripts are written in Python 3.11 or above (preferred; required if funded by a CPO grant), NCL, or R
  • All of my scripts are in the diagnostics/[POD short name] subdirectory, and include a main_driver script, template html, and settings.jsonc file
  • I have made corresponding changes to the documentation in the POD's doc/ subdirectory
  • I have requested that the framework developers add packages required by my POD to the python3, NCL, or R environment yaml file if necessary, and my environment builds with conda_env_setup.sh
  • I have added any necessary data to input_data/obs_data/[pod short name] and/or input_data/model/[pod short name]
  • My code is portable; it uses MDTF environment variables, and does not contain hard-coded file or directory paths
  • I have provided the code to generate digested data files from raw data files
  • Each digested data file generated by the script contains numerical data (no figures), and is 3 GB or less in size
  • I have included copies of the figures generated by the POD in the pull request
  • The repository contains no extra test scripts or data files

Generated plot generated from sample data GFDL-CM3_historical_r1i1p1.*.day.nc

The plots look like this (if changing the hyperlink "plot" to actual PNG images) - these are generated out of a small sample so the patterns are not representative:

Screenshot 2024-05-19 at 10 15 53 PM

@wrongkindofdoctor wrongkindofdoctor added the diagnostic Issue pertains to a contributed diagnostic label May 20, 2024
dz = 1000 # TODO Variable to set earlier?
hmax = -SCALE_HEIGHT * np.log(gridfilled_dataset[plev_name].min() / P_GROUND)
kmax = int(hmax // dz) + 1
original_pseudoheight = convert_hPa_to_pseudoheight(original_grid[plev_name]).rename("height")

Check notice

Code scanning / CodeQL

Unused local variable

Variable original_pseudoheight is not used.
def plot_and_save_figure(seasonal_average_data, analysis_height_array, plot_dir, title_str, season,
xy_mask=None, yz_mask=None):
if xy_mask is None:
xy_mask = np.zeros_like(seasonal_average_data.u_baro)

Check notice

Code scanning / CodeQL

Unused local variable

Variable xy_mask is not used.
else:
yland, xland = np.where(xy_mask)
if yz_mask is None:
yz_mask = np.zeros_like(seasonal_average_data.zonal_mean_u)

Check notice

Code scanning / CodeQL

Unused local variable

Variable yz_mask is not used.
Copy link
Copy Markdown
Contributor

@wrongkindofdoctor wrongkindofdoctor left a comment

Choose a reason for hiding this comment

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

@csyhuang. Thanks for the preliminary PR. The POD code looks okay from a technical standpoint. I just have a couple of change requests for your custom environment file and runtime configuration file that you can include with your final submission.

@@ -0,0 +1,128 @@
// Configuration for MDTF-diagnostics driver script self-test.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please move this file to the diagnostics/finite_amplitude_wave_diag directory before the final submission.

Copy link
Copy Markdown
Author

@csyhuang csyhuang May 29, 2024

Choose a reason for hiding this comment

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

@wrongkindofdoctor Hi Jess, thank you for reviewing the pull request and make changes to main accordingly! With the merged commits, ./mdtf -f src/default_finite_amplitude_wave_diag.jsonc -v no longer works. How shall I run my POD from this branch end-to-end on my machine?

(I created a new branch finite_amplitude_wave_diag_fcca20a_prelimPR from the current one, merged the new commits from main, and did the 2 changes you mentioned here. After testing them end-to-end I'll merge them to this branch.)

Thanks for your kind help!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@csyhuang Your runtime configuration file needs to be updated to match the new template format here: https://github.com/NOAA-GFDL/MDTF-diagnostics/blob/main/templates/runtime_config.jsonc.

Note that the POD_LIST is now a separate entry, WORKING_DIR is now WORK_DIR, and several of the previous settings have been removed.

Also, given that the main branch is under active development as we resolve outstanding issues with the preprocessor, your POD is not required to run with the current state of the framework. Just make sure that you have the required documentation in your RST file along with requested the changes to your test files, and the framework team will finalize testing at GFDL. We will reach out if we encounter technical issues in your POD.

Copy link
Copy Markdown
Author

@csyhuang csyhuang May 30, 2024

Choose a reason for hiding this comment

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

@wrongkindofdoctor Hi Jess, thank you for the instructions. I have a few follow-up questions:

In runtime_config.jsonc, there are two items not present in previous commits which I don't know how to configure:

  • case_list
  • DATA_CATALOG

I looked at the instructions in README.md and tried running tools/catalog_builder/catalog_builder.py but encounter some errors (e.g. failed to import src due to missing path - shall I submit an issue ticket about these?). Is the script specific to GFDL environment?

Also, given that the main branch is under active development as we resolve outstanding issues with the preprocessor, your POD is not required to run with the current state of the framework. Just make sure that you have the required documentation in your RST file along with requested the changes to your test files, and the framework team will finalize testing at GFDL. We will reach out if we encounter technical issues in your POD.

In that case, shall I keep the script files on this branch untouched, and notify you again when I have all documentation finished (+ made the changes you mentioned)? Please let me know how to proceed. Thanks! 😄

Copy link
Copy Markdown
Contributor

@wrongkindofdoctor wrongkindofdoctor May 30, 2024

Choose a reason for hiding this comment

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

No need. Those modules are needed for building GFDL datasets, and I haven't solved the import issue yet. I have commented out the problem lines and pushed the workaround to the main branch. Once you pull in the changes, you can use the catalog builder to generate intake catalogs for CMIP or CESM data.

@@ -0,0 +1,19 @@
name: _MDTF_finite_amplitude_wave_diag
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

please remove this file from the PR. The gridfill, bottleneck, and falwa packages are now included in the enve_python3_base.yml file.

- bottleneck
- pip=23.3.1
- pip:
- git+https://github.com/csyhuang/hn2016_falwa.git
Copy link
Copy Markdown
Author

@csyhuang csyhuang Aug 16, 2024

Choose a reason for hiding this comment

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

@wrongkindofdoctor Hi Jess, we figured out a solution to upgrade falwa to python 3.12 by migrating the F2PY build to meson. We tested on both Mac and Linux machine that the new installation procedures work. Do you have a chance to test if you can get our package installed via

- pip:
    - git+https://github.com/csyhuang/hn2016_falwa.git

We can do deployment on PyPI if cloning from GitHub is not preferred.

Thanks and we look forward to your update!

Clare

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@csyhuang Excellent! I'm glad you were able to find a replacement for F2PY. Yes, please push the update to PyPI and reference the package instead of the git repo in the environment file.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@wrongkindofdoctor I've just pushed an update to the PR branch and changed the line to - falwa==2.0.0. Thanks Jess!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@csyhuang Thanks! Please move your environment file and runtime config file from /src to your POD directory; I will add falwa and the dependencies back to the python3 base env files and verify that they build in the in the CI.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@wrongkindofdoctor Sure! I've moved the files to diagnostics/finite_amplitude_wave_diag. Please keep us updated after you tested it! Thanks Jess!

Copy link
Copy Markdown
Contributor

@wrongkindofdoctor wrongkindofdoctor Aug 19, 2024

Choose a reason for hiding this comment

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

@csyhuang The environments have been tested with the falwa package updates and merged into the python3_base env files.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@wrongkindofdoctor Glad that it works! Thanks Jess! We'll update you after finishing the rest of our POD!

@bitterbark
Copy link
Copy Markdown
Collaborator

Is the only conflict in the README.md? It looks like you put in your new lines and meanwhile someone else has done theirs. If you could get the new version and add your lines so they don't conflict with the changes, it might work.

@csyhuang
Copy link
Copy Markdown
Author

@bitterbark Hi Dani, sorry for the late reply as I have been doing long-distance travel in the past few weeks! ✈️ There are a few things left in this PR which still requires some work:

  1. Data input using data catalog has not been properly configured yet, mainly because I haven't found a dataset without errors that satisfies the vertical resolution requirement of my POD (one dataset from GFDL-CM4 fulfilled the resolution requirement but its vertical coordinates were inverted by mistake - I contacted Wenhao in some previous email regarding this)
  2. Predigest data has not been included in this POD yet, mainly because I am having trouble accessing UChicago's server - resolution is still pending
  3. Scientific description of the POD - we'll finish that ASAP

I will try to resolve all conflicts with main branch this weekend and keep you updated. Thanks a lot!

@bitterbark
Copy link
Copy Markdown
Collaborator

bitterbark commented Jun 3, 2025 via email

@yihungkuo
Copy link
Copy Markdown
Contributor

@aradhakrishnanGFDL Hi Aparna, here's the PR in question. Thanks.

@csyhuang
Copy link
Copy Markdown
Author

csyhuang commented Nov 3, 2025

@bitterbark Dani, sorry for missing your previous message! Vertical resolution similar to ERA-5, which can resolve stratosphere up to 1hPa shall work.

@aradhakrishnanGFDL For the purpose of building the POD, may I get access to some GFDL-CM4 data that has has correct postprocessing done? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

diagnostic Issue pertains to a contributed diagnostic

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants