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

Restarted from enhancement to aggregation script and small fixes #185

Open
wants to merge 20 commits into
base: develop
Choose a base branch
from

Conversation

scrasmussen
Copy link
Member

@scrasmussen scrasmussen commented Apr 8, 2024

TYPE: bugfix

KEYWORDS: precipitation, restarts, ideal tests, Python requirements

SOURCE: Soren Rasmussen, NCAR

DESCRIPTION OF CHANGES:

  • fixes to aggregation process to prevent mixing output data from runs started at different times:
    • added restarted_from global attribute to NetCDF files
    • before aggregating output files, aggregation script will:
      1. delete aggregated files created on and after restarted_from date
      2. if restarted_from is equal to Not Restarted, delete all aggregated files
  • small fixes to scripts that generate the ideal testing files icar_options.nml, init.nc and forcing.nc
  • git will ignore files produced by ideal test
  • added instructions and files for installing ICAR Python tool dependencies
  • Tiedke to Tiedtke spelling fixes
  • removed .travis.yml since Travis CI is no longer used
  • this PR has same changes that are in Issue 182 Fix: Xarray Future Warning #183

ISSUE: Fixes #180 and negative precip

TESTS CONDUCTED: Used the ideal testing files to do runs testing the new features in the aggregate script. Tested behavior of script with different restarted_from NetCDF values. Ran new aggregate script on old outputted files that had negative precip and it fixed the issue.

NOTES: Optional, as appropriate. Delete if not used. Included only once for
new features requiring several merge cycles. Changes to default behavior are
also note worthy.

Checklist

@scrasmussen scrasmussen force-pushed the enhancement/aggregate-precip-fix-and-cleanup branch from 70cf156 to de71135 Compare April 9, 2024 15:10
Copy link
Member

@gutmann gutmann left a comment

Choose a reason for hiding this comment

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

I think we agreed that matrix-nio could be removed from the requirements/environment, can you do that and anything else you made a note of when we talked last week?

helpers/environment.yml Outdated Show resolved Hide resolved
helpers/requirements.txt Outdated Show resolved Hide resolved
@scrasmussen
Copy link
Member Author

Hi @gutmann, sorry for the slow changes, took three days of PTO so was a bit slow getting to this but the things we talked about should be encapsulated in these new commits. I changed the README to describe the additional Python package requirements.

I should note that the Bunch package is 12 years old and doesn't support Python 3. I can look at updating the make_domain.py script if you'd like.

@gutmann
Copy link
Member

gutmann commented Apr 19, 2024

Thanks, if I recall correctly, "bunch" is a ~10 line python module... either it could be updated or we can just drop it. I suspect none of the scripts that are actively used anymore still use it. Let's just move all the scripts that do rely on it to some sort of deprecated archive folder for reference. I realize we could just delete them and they will still exist in git, but nobody would find them if we do that, and they could still be useful for someone to modify for future projects.

@gutmann
Copy link
Member

gutmann commented Apr 19, 2024

Weird, looking at that bunch repo, it has a python3 compatibility, does that not work? Also, funny that the core 10-line functionality has now ballooned into 400+ lines of code with all the comments, yaml/json support, etc. Useful, but then with just 10-lines I knew what it was doing 🙃

@scrasmussen scrasmussen force-pushed the enhancement/aggregate-precip-fix-and-cleanup branch from 4290db1 to e26251f Compare April 23, 2024 18:59
@scrasmussen
Copy link
Member Author

PR should be good to go, here is a summary of changes since there were a lot of things stuck into this one:

  • Output Changes
    • Added restarted_from attribute
    • Bundled init statements for output/restart objects into single init
      subroutine call: main/driver.f90:75, output_obj:9-19
  • Aggregate Scripts
    • Added restarted_from attribute to NetCDF files
    • aggregate_parallel_files.py:246 delete if after restarted_from data,
      else just print a warning
  • Other Python Scripts
    • README with dependency and setup instructions for Python Scripts
    • Pandas dependency removed
    • Small fixes to run with updated packages
    • Will use local Bunch and other packages in helpers/lib directory
  • removed Travis CI file
  • Tiedtke name fix

…l attribute of the NetCDF file, removes aggregated files newer than than that date if it exists. Otherwise remove all aggregated files before aggregation. This prevents negative precip from sneaking into output, due to output differences from different restarted from dates
…otlib. Updating README on how to install older packages Bunch and nio
…iles not from a restarted run or do not have the restarted_from attribute print a note of this, rather than delete existing aggregated files
…ns that need to be called, cleaning up the driver code a bit. Old output init is renamed to init_variables
…ecated units package.

Small fixes to other python scripts so they run.
README updated to say that the Nio package is not installed as a dependency here
@scrasmussen scrasmussen force-pushed the enhancement/aggregate-precip-fix-and-cleanup branch from 9fdcf3b to c76f98d Compare June 4, 2024 16:47
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.

None yet

2 participants