Skip to content

Add explicit weights_file configuration option - #98

Closed
toakinrele wants to merge 3 commits into
CIROH-UA:mainfrom
toakinrele:issue-45-weights-file
Closed

toakinrele wants to merge 3 commits into
CIROH-UA:mainfrom
toakinrele:issue-45-weights-file

Conversation

@toakinrele

Copy link
Copy Markdown
Collaborator

Summary

Adds an explicit weights_file configuration option so precomputed forcing weights can be provided separately from gpkg_file.

Previously, precomputed weights could be passed through gpkg_file as a workaround. This change separates the two inputs while preserving the existing GeoPackage-based behavior when weights_file is not provided.

Changes

  • Added support for weights_file in the forcing configuration.
  • Uses precomputed weights when weights_file is provided.
  • Falls back to the existing gpkg_file workflow when no weights_file is provided.
  • Keeps gpkg_file available for operations that require the hydrofabric.

Testing

Tested using cat-1555522_subset.gpkg and precomputed weights.parquet.

Compared forcing output from the existing GeoPackage-based workflow against the new explicit weights_file workflow:

  • Same output shape: True
  • Exact DataFrame equality: True
  • Maximum absolute difference across all forcing variables: 0.0
  • Precomputed weights successfully bypassed weight recalculation.

Closes #45

@quinnylee

Copy link
Copy Markdown
Collaborator

@toakinrele Thanks for the PR! Could you push the weights.parquet file into the docs directory? (if it's reasonably small)?

@toakinrele

Copy link
Copy Markdown
Collaborator Author

Thanks! The file is only 3.1 KB, so I added weights.parquet to docs/examples/cfe-nom_example/ and pushed it to the PR.

@quinnylee quinnylee left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Overall functionality looks good! Pytests are passing locally.

Only thing I would ask for is maybe a unit test in test_forcingprocessor.py to test the weights override functionality

@JordanLaserGit JordanLaserGit left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@toakinrele Thanks for this PR! The changes here look good though a couple things before we merge.

There are a couple checks to see if plotting will proceed and one of them will be influenced by these changes.
Here:

ii_plot = conf.get("plot", False)
if ii_plot:
if data_source == "channel_routing" or data_source == "troute_restarts":
raise RuntimeError(
"Plotting not supported for channel routing or restart processing."
)

and here:
if ii_plot:
if gpkg_files[0].endswith(".parquet"):
print(f"Plotting currently not implemented for parquet, need geopackage")
else:
if len(gpkg_files) > 1:
raise Warning(f"Plotting only the first geopackage {gpkg_files[0]}")

Ideally we perform this check in only one spot, and it seems like that first location is ideal. Could you combine these with the logic you're implementing in this PR?

Also, this branch is from your forked repo, so please clone the original repo, make a branch, and PR that one. This allows the git actions tests to run that require the AWS secrets stored in the CIROH-UA repo.

@toakinrele

Copy link
Copy Markdown
Collaborator Author

I’ve addressed the review comments and added the weights_file override test. The new test passes locally, along with the plotting test and the local output tests.

I also tried pushing the branch directly to CIROH-UA/forcingprocessor as requested, but I’m getting:

Permission to CIROH-UA/forcingprocessor.git denied to toakinrele.

Could you grant me permission to push a branch to the upstream repository, or create the branch there so I can push the changes and allow the AWS-dependent GitHub Actions to run?

@JordanLaserGit

Copy link
Copy Markdown
Collaborator

@arpita0911patel could we get @toakinrele permissions?

@arpita0911patel

Copy link
Copy Markdown
Member

Thank you @toakinrele for this PR! Provided write permission to you for this repo.

@JordanLaserGit

Copy link
Copy Markdown
Collaborator

Closing this pr as this work has moved to #99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

Make weight file hack an explicit option

4 participants