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

Feature/top heaviness metric #229

Merged

Conversation

jiacheng-atmos
Copy link
Contributor

@jiacheng-atmos jiacheng-atmos commented May 24, 2021

Description
A new POD - top-heaviness metric is introduced.

How Has This Been Tested?
Please describe the tests that you ran to verify your changes in enough detail that
someone can reproduce them. Include any relevant details for your test configuration
such as the Python version, package versions, expected POD wallclock time, and the
operating system(s) you ran your tests on.

Checklist:

  • I have reviewed my own code to ensure that if follows the POD development guidelines
  • The script are written in Python 3.6 or above (preferred; required if funded by a CPO grant), NCL, or R
  • All of my scripts are in the diagnostics/ subdirectory, and include a main_driver script template html, and settings.jsonc
  • The main_driver script header has all of the information in the POD documentation, excluding the "More about this diagnostic" section
  • The POD directory and html template have the same short name as my POD
  • The html template has a 1-paragraph synopsis of the POD and links to the main documentation
  • If applicable, I've added a .yml file to src/conda, and my environment builds with conda_env_setup.sh
  • The POD scripts do not access the internet or networked resources
  • I have commented the code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation in the POD's doc/ subdirectory
  • I have created the directory input_data/obs_data/[pod short name]
  • My code is portable; it uses MDTF environment variables, and does not contain hard-coded file or directory paths
  • I have added information about the raw data files to the documentation
  • [] 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
  • The repository contains no extra test scripts or data files
  • My branch is up-to-date with the NOAA-GFDL develop branch, and all merge conflicts are resolved

@wrongkindofdoctor wrongkindofdoctor removed the request for review from tsjackson-noaa October 4, 2021 15:15
Copy link
Collaborator

@aradhakrishnanGFDL aradhakrishnanGFDL left a comment

Choose a reason for hiding this comment

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

@jiacheng-atmos Thank you for your contributions.

The testing routines seem to have caught a formatting error in the settings.jsonc. Please double check and provide an updated settings.jsonc, as needed.

diagnostics/top_heaviness_metric/settings.jsonc

Please check the data fields and also ensure the closing braces are all provided at the end of the jsonc. I find the online json linter useful for these purposes.

ERROR: Couldn't parse the settings.jsonc file for POD 'top_heaviness_metric': Syntax error in 'settings.jsonc': Expecting ',' delimiter: line 12 column 16 (char 783).
32

@jiacheng-atmos
Copy link
Contributor Author

@jiacheng-atmos Thank you for your contributions.

The testing routines seem to have caught a formatting error in the settings.jsonc. Please double check and provide an updated settings.jsonc, as needed.

diagnostics/top_heaviness_metric/settings.jsonc

Please check the data fields and also ensure the closing braces are all provided at the end of the jsonc. I find the online json linter useful for these purposes.

ERROR: Couldn't parse the settings.jsonc file for POD 'top_heaviness_metric': Syntax error in 'settings.jsonc': Expecting ',' delimiter: line 12 column 16 (char 783). 32

Thank you for your advice. I have updated the .jsonc file. Issues related to that seem to be resolved by now. Additionally, may I ask how to provide the code to generate digested data files?
"I have provided the code to generate digested data files from raw data files"

@jiacheng-atmos
Copy link
Contributor Author

Thank you for your advice. I have updated the .jsonc file. Issues related to that seem to be resolved by now.

@jiacheng-atmos
Copy link
Contributor Author

@jiacheng-atmos Thank you for your contributions.

The testing routines seem to have caught a formatting error in the settings.jsonc. Please double check and provide an updated settings.jsonc, as needed.

diagnostics/top_heaviness_metric/settings.jsonc

Please check the data fields and also ensure the closing braces are all provided at the end of the jsonc. I find the online json linter useful for these purposes.

ERROR: Couldn't parse the settings.jsonc file for POD 'top_heaviness_metric': Syntax error in 'settings.jsonc': Expecting ',' delimiter: line 12 column 16 (char 783). 32

This issue has been resolved

@wrongkindofdoctor wrongkindofdoctor changed the base branch from develop to main June 17, 2022 14:22
@wrongkindofdoctor
Copy link
Collaborator

@jiacheng-atmos As with the PR #250, I've switched the target branch to main. I'll ping @jkrasting for a followup review, and we'll see we can get this merged soon. Please provide any observational datasets required for this POD by sharing the link to the server location, or emailing me a tarball with the files.

@aradhakrishnanGFDL
Copy link
Collaborator

@jiacheng-atmos As with the PR #250, I've switched the target branch to main. I'll ping @jkrasting for a followup review, and we'll see we can get this merged soon. Please provide any observational datasets required for this POD by sharing the link to the server location, or emailing me a tarball with the files.

@wrongkindofdoctor,few questions to help review this PR and to document discussion TODOs for mini-leads.

1- what's the recommended place to provide an example input settings json used while testing a POD (by POD developer)?

2- Is the POD README file a better place to document the required datasets (OBS in particular)?

3- @jiacheng-atmos also had a question on where to push the script to generate pre-digested obs data? I think this is an excellent question and I'd like to know if there is a recommended path for this as well. If not, we can discuss this with mini-leads at a future date.

@wrongkindofdoctor
Copy link
Collaborator

wrongkindofdoctor commented Jun 23, 2022

@aradhakrishnanGFDL
1.If you mean a modified runtime default_tests.jsonc file, it would be useful to have a copy called [pod name]_runtime.jsonc with the sample datasets and settings used for testing in the diagnostics/[pod name] directory
2. The POD README file is probably a better place for required dataset documentation than the code comments
3. The script to generate the pre-digested obs data can be placed in the diagnostics/[pod name] directory

@aradhakrishnanGFDL
Copy link
Collaborator

@aradhakrishnanGFDL 1.If you mean a modified runtime default_tests.jsonc file, it would be useful to have a copy called [pod name]_runtime.jsonc with the sample datasets and settings used for testing in the diagnostics/[pod name] directory

Yes. Let's discuss this at mini-leads and add it to the checklist. For this POD, @jiacheng-atmos , if you have the runtime default_tests.jsonc you used for your tests, please consider (not a requirement at this time) pushing a copy per @wrongkindofdoctor's suggested path/filename.

  1. The POD README file is probably a better place for required dataset documentation than the code comments

Great. @jiacheng-atmos can you please update the README based on the above note?

  1. The script to generate the pre-digested obs data can be placed in the diagnostics/[pod name] directory

@jiacheng-atmos Since you asked about the pre-digested obs data generation script locations, please push it to diagnostics/[pod name] directory if it's not there already.

Many thanks for your contributions.

@wrongkindofdoctor wrongkindofdoctor merged commit bce986e into NOAA-GFDL:main Aug 17, 2022
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.

5 participants