Skip to content

Test cice warnings#26

Open
NickSzapiro-NOAA wants to merge 14 commits into
developfrom
test_cice_warnings
Open

Test cice warnings#26
NickSzapiro-NOAA wants to merge 14 commits into
developfrom
test_cice_warnings

Conversation

@NickSzapiro-NOAA

Copy link
Copy Markdown
Owner

Commit Queue Requirements:

  • This PR addresses a relevant WM issue (if not, create an issue).
  • All subcomponent pull requests (if any) have been reviewed by their code managers.
  • Run the full Intel+GNU RT suite (compared to current baselines), preferably on Ursa (Derecho or Hercules are acceptable alternatives). Exceptions: documentation-only PRs, CI-only PRs, etc.
    • Commit log file w/full results from RT suite run (if applicable).
    • Verify that test_changes.list indicates which tests, if any, are changed by this PR. Commit test_changes.list, even if it is empty.
  • Fill out all sections of this template.

Description:

Commit Message:

* UFSWM - 
  * AQM - 
  * CDEPS - 
  * CICE - 
  * CMEPS - 
  * CMakeModules - 
  * UFSATM - 
    * ccpp-physics - 
    * atmos_cubed_sphere - 
  * GOCART - 
  * MOM6 - 
  * NOAHMP - 
  * WW3 - 
  * fire_behavior
  * stochastic_physics - 

Priority:

  • Critical Bugfix: Reason
  • High: Reason
  • Normal

Git Tracking

UFSWM:

  • Closes #

Sub component Pull Requests:

  • AQM:
  • CDEPS:
  • CICE:
  • CMEPS:
  • CMakeModules:
  • UFSATM:
    • ccpp-physics:
    • atmos_cubed_sphere:
  • GOCART:
  • MOM6:
  • NOAHMP:
  • WW3:
  • fire_behavior:
  • stochastic_physics:
  • None

UFSWM Blocking Dependencies:

  • Blocked by #
  • None

Documentation:

  • Documentation update required.
    • Relevant updates are included with this PR.
    • A WM issue has been opened to track the need for a documentation update; a person responsible for submitting the update has been assigned to the issue (link issue).
  • Documentation update NOT required.
    • Explanation:

Changes

Regression Test Changes (Please commit test_changes.list):

  • PR Adds New Tests/Baselines.
  • PR Updates/Changes Baselines.
  • No Baseline Changes.

Input data Changes:

  • None.
  • PR adds input data.
  • PR changes existing input data.

Library Changes/Upgrades:

  • Required
    • Library names w/versions:
    • Git Stack Issue (JCSDA/spack-stack#)
  • No Updates

Testing Log:

  • RDHPCS
    • Orion
    • Hercules
    • GaeaC6
    • Derecho
    • Ursa
  • WCOSS2
    • Dogwood/Cactus
    • Acorn
  • CI
  • opnReqTest (complete task if unnecessary)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a Python script for monitoring compiler warnings, updates the CICE submodule to point to the Consortium repository, and refactors regression test scripts to reduce duplication. It also adds several new coupled model test configurations and updates CICE namelist parameters. Feedback focuses on improving the robustness of the warning check script by avoiding hardcoded branch names and removing redundant variable assignments in the new test scripts that could lead to incorrect thread counts.

sys.exit(1)

log_directory = sys.argv[1]
base_branch = "develop"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The base branch is hardcoded to develop. This script will produce incorrect results if the pull request targets a different branch (e.g., a release branch or a hotfix branch). It is better to pass the base branch as an argument or use the GITHUB_BASE_REF environment variable to make the script more robust and reusable across different target branches.

Suggested change
base_branch = "develop"
base_branch = sys.argv[2] if len(sys.argv) > 2 else "develop"

Comment on lines +21 to +22
export atm_omp_num_threads=$THRD_cpl_unstr
export med_omp_num_threads=$atm_omp_num_threads

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

These lines are redundant and potentially buggy. Line 21 overwrites the atm_omp_num_threads value set on line 11 with the default THRD_cpl_unstr. If the intention of this test was to use the MPI-specific thread count (THRD_cpl_unstr_mpi), line 21 effectively reverts that change. Removing line 21 allows line 22 to correctly use the value assigned on line 11.

export med_omp_num_threads=$atm_omp_num_threads

Comment on lines +23 to +24
export atm_omp_num_threads=$THRD_cpl_unstr
export med_omp_num_threads=$atm_omp_num_threads

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Similar to other test scripts in this PR, line 23 overwrites the specific atm_omp_num_threads setting from line 11 with a global default. This redundancy should be cleaned up to ensure that if THRD_cpl_unstr_mpi is ever changed to differ from the default, the test actually uses the intended value.

export med_omp_num_threads=$atm_omp_num_threads

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.

1 participant