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

PR WIP - CatFIM 2.1: Add Alaska, reconcile sites, fix CatFIM V2.0 bugs #1285

Open
wants to merge 136 commits into
base: dev
Choose a base branch
from

Conversation

RobHanna-NOAA
Copy link
Contributor

@RobHanna-NOAA RobHanna-NOAA commented Sep 12, 2024

There is a very wide range of fixes in this PR as we made a CatFIM 2.1 (informal name). All of this was managed via a card we had called an "EPIC" card #1182 to manage all of the changes.

For a list of all of the changes, see that card.
Item numbers fixed from that card are and may also picked up their own card:

  • 10 : 1311: Add HUC list functionality to CatFIM
  • 11: (no card): Finish the new "step" system, allowing us to jump parts of the code.
  • 13: (no card): Not complete but lots of progress on cleaning up doc strings.
  • 14: 1272: Review CatFIM Datum Adjustments.
  • "Note from Derek": No card, but done and in this release.
  • 15: 1273: Review 4.5.2.11 flow and stage based error log results
  • 16: 1274: HV dropping CatFIM records inc Sea level sites (and move HV ahps_restricted_sites system to CatFIM code).
  • 17: (no card): Ensure that the stage_based_catfim_sites.csv file has the 'nws_lid' column renamed to 'ahps_lid'.
  • 19: 1277 : CatFIM - issues with new docker image
  • 20: Review sites that were dropped since 4.4.0.0 to 4.5.2.11: Fixed via a number of other tasks and bug fixes.
  • 22: 1282 : Make a CatFIM folder to store the CatFIM scripts!
  • 23: 1279 ; Hawaii points disappeared from stage-based CatFIM in release 4.5.2.11.
  • 24 : (no card) : Ensure we compare mapped = Yes from out site list, and ensure there is at least one rec in library file.
  • 26: 1336: Fix duplicate LID records.

Other fixes:

  • A significant bug was found that existed back at fim 4.4.0.0 which was not calculating interval layers correctly. Now fixed. This will likely add a large handful of more layers in the final output (TBD).
  • Added a new tool called the catfim sites compare tool. This allows us to compare outputs from one version to another. ie) compare sites or libraries from 4.4.0.0 to 4.5.2.11, etc.

Note: This is now a bug in the linting system and is being addressed. The Github version of the linting tools do not match the versions in our pyproject.toml and the fix appears to be harder than it seems. It is WIP.

Additions

  • tools
    • catfim
      • README.md: Some notes about the system. It is incomplete but lets us track a bunch of info about the product
      • catfim_sites_compare.py: As mentioned above, comparing outputs of versions.
      • images
        • screenshot_vis_settings.jpg: in support of the catfim README.md file
          - stage_based_ahps_restricted_sites.csv: As described above.

Files Renamed or Moved

  • tools:
    • .env.template renamed to catfim.env.template
    • generate_categorical_fim.py : moved into a catfim subfolder.
    • generate_categorical_fim_flows.py: moved into a catfim subfolder.
    • generate_categorical_fim_mapping.py: moved into a catfim subfolder.

Changes

  • .gitignore: to not load the new stage_based_ahps_restricted_sites.csv file. Also blocks load of a catfim env template file.
  • pyproject.toml: WIP progress fixing linting issues as mentioned above.
  • src/utils/fim_logger.py: Small test and pattern changes
  • tools
    • catfim
      • generate_categorical_fim.py, generate_categorical_fim_flows.py and generate_categorical_fim_mapping.py: as described above.

Removals

  • tools\.gitignore: not applicable after moving config.env to input parameters

Testing

A very, very wide amount of testing was done throughout the processes. The logging system and some logging additions allowed for tracing of problems and data issues. It is currently running the stage based production outputs and will do flow based shortly.

Deployment Plan (For developer use)

How does the changes affect the product?

  • Code only?
  • If applicable, has a deployment plan be created with the deployment person/team?
  • Require new or adjusted data inputs? Does it have start, end and duration code (in UTC)?
  • If new or updated data sets, has the FIM code been updated and tested with the new/adjusted data (subset is fine, but must be a subset of the new data)?
  • Require new pre-clip set?
  • Has new or updated python packages?

Issuer Checklist (For developer use)

You may update this checklist before and/or after creating the PR. If you're unsure about any of them, please ask, we're here to help! These items are what we are going to look for before merging your code.

  • Informative and human-readable title, using the format: [_pt] PR: <description>
  • Links are provided if this PR resolves an issue, or depends on another other PR
  • If submitting a PR to the dev branch (the default branch), you have a descriptive Feature Branch name using the format: dev-<description-of-change> (e.g. dev-revise-levee-masking)
  • Changes are limited to a single goal (no scope creep)
  • The feature branch you're submitting as a PR is up to date (merged) with the latest dev branch
  • problems in system identified - pre-commit hooks were run locally
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • CHANGELOG updated with template version number, e.g. 4.x.x.x
  • Add yourself as an assignee in the PR as well as the FIM Technical Lead

Merge Checklist (For Technical Lead use only)

  • Update CHANGELOG with latest version number and merge date
  • Update the Citation.cff file to reflect the latest version number in the CHANGELOG
  • If applicable, update README with major alterations

@EmilyDeardorff EmilyDeardorff linked an issue Nov 5, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment