Skip to content

Conversation

@ceblanton
Copy link
Collaborator

@ceblanton ceblanton commented Jul 11, 2025

Ian: @ceblanton and @ilaflott collab, i'll be summarizing what's up here soon

the main mission- climatology calculations

  • new CLI routine and submodule, fre app gen-time-averages-wrapper / generate_time_averages.generate_wrapper, which generates an output climatology
  • new CLI command and submodule, fre app combine-time-averages / generate_time_averages.combine, combines files holding individual monthly averages into one for the climatology generation
  • tests for each: generate_time_averages/tests/test_wrapper.py, generate_time_averages/tests/test_combine.py
  • 61117 lines of cdl over 34 cdl files containing test data

small side quests, tweaks

  • gfdl_msd_schemas update (removes do_timeavgs as option, superceded by climatology), tweaks to testing yaml configs in this repo removing the do_timeavgs key
  • moved fre/app/tests/test_mask_atmost_plevel.py to fre/app/mask_atmos_plevel/tests, and gave it an __init__.py for coverage/linting

bigger side quests, efforts

general fre/app/generate_time_averages improvements

  • typing additions
  • pylint oriented edits for all files based on current repo standard
  • improved exception handling and tests for it
  • rewritten tests/test_generate_time_averages from the ground-up, heavily leveraging pytest.parameterize, increased clarity into testing approach, etc.
  • small tests covering previous uncovered lines, and small tweaks to certain logic structures to enhance testability
  • some print statements in non-testing code still remained, this was fully removed and replaced entirely with fre_logger usage

.github/workflows revamped:

  • channel configuration now explicit + equivalent in create_test_conda_env, build_conda, publish_conda workflows, set_channel_priority is now strict
  • workflows also factorized into more numerous, smaller steps for easier nav
  • conda-forge::conda-verify and conda-forge::anaconda-client don't really do anything since they're really for uploading to conda channels r and main, and so are removed from publish_conda
  • fixed a bug causing the spell_check workflow to redundantly run twice each push
  • create_test_conda_env now explicitly activates the fre-cli environment after conda init, which is enabled by sourcing conda/etc/profile.d/conda.sh
  • all CI/CD containers in this repo no longer use ghcr.io/noaa-gfdl/fre-cli:miniconda24.7.1_gcc14.2.0, and instead now use ghcr.io/noaa-gfdl/fre-cli:miniconda24_gcc14_v2

packaging changes:

  • pyproject.toml list of deps changed to include analysis_scripts, cftime, netCDF4, numpy and pyyaml, all of which are explicitly called in the body of the code and are pip/python packages. also removes pylint and pytest under this section and puts them under the test-specific reqs
  • meta.yaml now defaults to conda-package version develop when there's no git tag, certain version reqs added to help with environment resolution, noaa-gfdl packages are all pinned to specific versions. we now explicitly ask for netCDF4=1.7.* and xarray>=2024.* to avoid certain behaviors assoc. with earlier versions calling test failures. Jinja2 no longer needs to be specifically version 3.0.*, just greater than 3. numpy set to 1.26.*, pylint, pytest and pytest-cov are added to the test: requires: section instead
  • environment.yaml tweaked similarly to above dependency lists, but still contains the testing requirements.
  • pylintrc now included in repo and is used for configuring the pylint standards, fail-under set to 7.5, max-positional-arguments increased to 6 from 5, workflow calls to pylint now use the config file instead

Checklist before requesting a review

  • I ran my code
  • I tried to make my code readable
  • I tried to comment my code
  • I wrote a new test, if applicable
  • I wrote new instructions/documentation, if applicable
  • I ran pytest and inspected it's output
  • I ran pylint and attempted to implement some of it's feedback
  • No print statements; all user-facing info uses logging module

@singhd789 singhd789 linked an issue Aug 25, 2025 that may be closed by this pull request
1 task
Chris Blanton and others added 3 commits August 25, 2025 17:44
- annual and monthly tests
- add reverse chdir to combine.py so the tests don't modify the working directory
@ceblanton ceblanton marked this pull request as ready for review August 26, 2025 15:21
Copy link
Member

@ilaflott ilaflott left a comment

Choose a reason for hiding this comment

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

solid effort again! i still think calling this a wrapper is implying the wrong thing about the code's structure, but aside from the name this is looking great

@ilaflott ilaflott requested review from mlee03 and singhd789 October 10, 2025 20:50
@ilaflott
Copy link
Member

@singhd789 @mlee03 i didn't quite get to all the feedback, but i got to a bunch of it and some of it i don't want to resolve without yee eyez

… in both combine and wrapper, and remove later redundant checks. add test for raising the exception
@mlee03
Copy link
Contributor

mlee03 commented Oct 14, 2025

@ilaflott I can't find the resolve conversation button lol. But all's good with me~
My comments were just annoying style change suggestions~

Copy link
Collaborator

@singhd789 singhd789 left a comment

Choose a reason for hiding this comment

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

My comments were addressed. Great work Ian

@ilaflott ilaflott merged commit acfb22d into main Oct 15, 2025
6 checks passed
@ilaflott ilaflott deleted the add-climo-wrapper branch October 15, 2025 17:08
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.

WF Testing: combine-timeavgs WF Testing: make-timeavgs

5 participants