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

Abstract network update #600

Merged
merged 56 commits into from
Feb 16, 2023

Conversation

shorvath-noaa
Copy link
Contributor

This update moves many of the common functions in HYFeatures_network and NHDNetwork objects into the parent Abstract_network object. It also generalizes functions when necessary to work with either (and hopefully new) networks.

The key differences in individual networks is how they read in files to create the self._dataframe (param_df) objects and various waterbody dataframes. Once these are created t-route should run the same for any network, creating the diffusive domain, initial warmstate, forcing data, etc, so those functions have been moved to the Abstract_network.

Additions

Removals

Changes

  • Moved creating diffusive domain, initial warmstate preprocess, and assemble_forcing functions to AbstractNetwork.
  • Generalized these functions when necessary to work with either network.

Testing

  1. Passed test on LowerColorado_TX with MC only and now outputs.
  2. Needs to be tested more with hybrid routing
  3. Needs to be tested with outputs turned on

Screenshots

Notes

  • Some features may still need to be moved around. Maybe creating diffusive domain should be done by individual networks since this involves reading in data files?
  • Waterbody links to gages for HYFeatures still needs to be worked out.
  • DA for HYFeatures still needs to be worked out.

Todos

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 ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@hellkite500
Copy link
Contributor

I've missed a few things related to these modules, but the last commit caught my eye. Why is column mapping explicit in the hy features network yet is still part of the abstract superclass construction?

Also, is there some reasoning as to why the utilities are not implemented as either instance or class methods?

@shorvath-noaa
Copy link
Contributor Author

shorvath-noaa commented Dec 19, 2022 via email

@shorvath-noaa
Copy link
Contributor Author

shorvath-noaa commented Dec 21, 2022

@hellkite500 I moved functions from ...preprocess.py files into the network files as inherent functions (830c6ef). Is that what you had in mind?

@shorvath-noaa shorvath-noaa marked this pull request as ready for review January 23, 2023 15:31
@shorvath-noaa
Copy link
Contributor Author

@hellkite500 @kumdonoaa I've updated this from a Draft to being ready for review. Please take one final look so I can merge it, thanks!


forcing_glob_filter = forcing_parameters.get("qlat_file_pattern_filter", "*.NEXOUT")

if forcing_glob_filter=="nex-*":
Copy link
Contributor

Choose a reason for hiding this comment

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

We might want a quick tag up to discuss this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing.

rconn,
wbody_conn,
reaches_bytw,
network,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a case to be made about keeping this call the same as before and expanding the args from the network at the calling code. It might be worth discussing the merits and/or a possible proxy to maintain the compute_nhd_routing_v02 backwards compatibility and possibly deprecate it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to discuss this further. Calling the main routing function has a couple levels and I can see the utility in keeping the bottom levels the same to maintain backwards compatibility. But I do think minimizing these inputs makes the code much more readable and would help with future maintenance.

@hellkite500
Copy link
Contributor

@shorvath-noaa Can you rebase this locally to resolve the conflicts? I left a few minor comments that can be addressed in follow up PR's, but we will need to get the conflicts resolved before this can merge.

shorvath-noaa and others added 26 commits February 15, 2023 20:56
…on into read_geo_file function for HyFeatures

return run_sets

def nex_files_to_binary(nexus_files, binary_folder):
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this is HYFeatures specific, and as such this function should exist in that sublcass, not the abstract class.

@@ -654,3 +658,201 @@ def _try_parsing_date(text):
"channel initial states complete in %s seconds."\
% (time.time() - start_time)
)

def build_forcing_sets(self,):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should probably look at factoring out the common code and the network specific code in this function.

@shorvath-noaa shorvath-noaa merged commit 01c713a into NOAA-OWP:master Feb 16, 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.

4 participants