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

Add support for NetCDF/Time-partitioned CSV QLAT inputs #636

Merged
merged 4 commits into from
Sep 9, 2023

Conversation

program--
Copy link
Contributor

@program-- program-- commented Aug 16, 2023

This PR adds support for reading NetCDF QLAT outputs from ngen via xarray.

The NetCDF schema for the new outputs are as follows:

netcdf 201512010000NEXOUT {
dimensions:
        feature_id = 3 ;
variables:
        string feature_id(feature_id) ;
                feature_id:description = "Contributing Nexus ID" ;
        double q_lateral(feature_id) ;
                q_lateral:description = "Runoff into channel reach" ;
                q_lateral:units = "m3 s-1" ;
        string time ;
                time:description = "Output timestamp for this contribution" ;
                time:standard_name = "time" ;
                time:format = "%Y%m%s%H%M" ;
data:

 feature_id = "nex-285284", "nex-285285", "nex-285286";

 q_lateral = 8.53977600158463, 7.45141160538848, 5.80901770862798;

 time = "201512010000";
}

The corresponding CSV header is feature_id, <time> (i.e. feature_id, 201512010000).

Related: NOAA-OWP/ngen#605, NOAA-OWP/ngen#612

Additions

  • Add NetCDF reading to both AbstractNetwork.read_file and HYFeaturesNetwork.read_file.
  • Adds timestamp parsing from file names for *NEXOUT glob filters.

Changes

  • Modifies build_qlateral_array when reading a q_lateral file to remove nex- from the prefix.

    This probably should follow the logic of trying to split on -, and remove everything prefixing it...

Testing

  • Adds a unit test for reading from ngen-outputted Q Lateral files (both CSV and NetCDF), and ensures forcing sets can be assembled.

Todos

  • Add logic for CSV heuristics Unneeded if relying on glob filter
  • Add unit tests

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • 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
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

@program--
Copy link
Contributor Author

program-- commented Aug 30, 2023

@shorvath-noaa if you get a chance, can you pull this PR and ensure the tests and in-memory df schema are as expected -- and maybe do a quick review in case there are things that I missed for this integration? Thanks!!

Comment on lines 524 to 527
qlats_df = pd.concat(
(nexuses_lateralflows_df.loc[int(k)].rename(v) for k,v in self.downstream_flowpath_dict.items() if k in nexuses_lateralflows_df.index.to_list()),
axis=1
).T
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be entering an infinite loop when I run t-route with *CHRTOUT files as forcing. If you change this to:
qlats_df = nexuses_lateralflows_df.rename(index=self.downstream_flowpath_dict)
it works with *CHRTOUT files and still passes your pytest file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, I think this works well. Implemented in ebfdd6d.

@program-- program-- marked this pull request as ready for review September 1, 2023 20:09
@program--
Copy link
Contributor Author

@shorvath-noaa any update on when this can be merged? NOAA-OWP/ngen#612 is soft-blocked on this (but no immediate rush). Thanks!!

@shorvath-noaa shorvath-noaa merged commit 0a49571 into NOAA-OWP:master Sep 9, 2023
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.

2 participants