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.

Sign up for GitHub

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

Jump to bottom

fix / feat: handle building gpkg crosswalk from flowpath-attributes #138

Merged
merged 3 commits into from
Aug 1, 2024
Merged

fix / feat: handle building gpkg crosswalk from flowpath-attributes #138

merged 3 commits into from
Aug 1, 2024

Conversation

aaraney
Copy link
Member

@aaraney aaraney commented Jul 30, 2024 •
edited
Loading

Newer HF versions now use:
flowlines instead of flowpaths
flowpath-attributes instead of flowpath_attributes
model-attributes instead of model_attributes
This just adds support for that.

Additions

  • Add support for hydrofabrics that use flowlines and flowpath-attributes

@aaraney aaraney added enhancement New feature or request ngen.cal Related to ngen.cal package labels Jul 30, 2024
@hellkite500
Copy link
Member

Which version uses which convention?

@aaraney
Copy link
Member Author

aaraney commented Jul 30, 2024 •
edited
Loading

2.1.1 uses flowpath-attributes - https://www.lynker-spatial.com/data?path=hydrofabric%2Fv2.1.1%2Fnextgen%2F

Everything <2.1.1 seems to use flowpath_attributes.

The beta of 2.2 doesnt include flowpath-attributes or flowpath_attributes. I assume that is b.c. they are focusing on the network and not the linked data at this point.

@aaraney
Copy link
Member Author

aaraney commented Jul 30, 2024

Re-running jobs on master b.c. I don't think they are failing from what i've added here.

@aaraney
Copy link
Member Author

aaraney commented Jul 30, 2024

Opened #139 to track the test failures. Confirmed the additions made here.

@aaraney
Copy link
Member Author

aaraney commented Jul 30, 2024 •
edited
Loading

Having chatted with @mikejohnson51 (correct me where i'm wrong), all HF releases >2.1 flowpaths -> flowlines and flowpath_attributes -> flowpath-attributes. Can you confirm that @mikejohnson51?

See here for an example of the new namespacing. Drop everything up-to and including the _ and you get the new layer (sqlite table) name (e.g. forcing-weights).

robertbartel
robertbartel previously approved these changes Aug 1, 2024
View reviewed changes
Copy link
Contributor

@robertbartel robertbartel left a comment

Choose a reason for hiding this comment

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

I had one comment on perhaps expanding some on the docstring to clarify what constitutes a "legacy hydrofabric," but this isn't critical and may be tricky given how hydrofabric data and schemas are currently versioned. So I'm good with it as-is.

python/ngen_cal/src/ngen/cal/ngen.py Outdated

#Read the calibration specific info
with open(self.realization) as fp:
data = json.load(fp)
self.ngen_realization = NgenRealization(**data)

@staticmethod
def _is_legacy_gpkg_hydrofabric(hydrofabric: Path) -> bool:
"""Return True if legacy hydrofabric."""
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be helpful to explain somewhere (i.e., either here, or with a reference here to wherever that is) what constitutes a "legacy hydrofabric." Are we talking about versions prior to v2.0? Versions >= 2.0 but older than the latest? I could make an educated guess, but, even assuming a high probability of correctness right now, that will get harder for someone in the future to do reliably.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion. Just updated the docstring to better reflect what it means to be legacy.

@aaraney
Copy link
Member Author

aaraney commented Aug 1, 2024

@robertbartel, just addressed your comment. Should be good to go now. Thanks!

@robertbartel robertbartel merged commit 6c25d62 into NOAA-OWP:master Aug 1, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertbartel robertbartel robertbartel approved these changes

@hellkite500 hellkite500 Awaiting requested review from hellkite500

Assignees
No one assigned
Labels
enhancement New feature or request ngen.cal Related to ngen.cal package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aaraney @hellkite500 @robertbartel