noaa-owp / ngen Goto Github PK

Forcing.h is unguarded, so it is possible to run into errors where a subclass might define it more than once and run into errors.

There should just be an #ifndef guard around it.

Short description explaining the high-level reason for the new issue.

Current behavior

README.md exists as per the template but doesn't reflect first sprint changes.

Expected behavior

README.md should reflect the project at the end of each sprint.

Steps to replicate behavior (include URLs)

Screenshots

I don't want to butt into #142 so I'll ask over here.

What's a basin? I didn't think we were working with that concept.

Per Fred's feedback of the nonlinear reservoir instantiation in hymod, clearer nonlinear reservoir param names are needed.

@jdmattern-noaa will follow-up with a PR to address this.

The realization constructor takes a long and constructs a parameter struct, implicitly casting to double. The struct should store a long, not a double.

@donaldwj please verify.

Short description explaining the high-level reason for the new issue.

Current behavior

Build tests exits Code 2

Expected behavior

All Tests: Build and Pass

Steps to replicate behavior (include URLs)

1.https://github.com/ZacharyWills/ngen/runs/577718969

Screenshots

This test code should be moved to the unit testing framework.

Improve how the CSV reader class (and associated usages of it) handle cases when an expected file doesn't exist. Right now the CSV reader itself doesn't fail, but in such cases it doesn't read anything either, leading to inconsistent behavior downstream in program execution.

A particular example was seen in unit testing for forcings. If the unit test executable is run from the main directory, rather than when inside the build directory, then a relative path needed for the ForcingTest.TestForcingDataRead test will not be correct, leading to a crash of the unit test executable.

More reservoir unit testing needed to directly test particular reservoir functions.

Expected behavior

We should have one or more TEST_F(ReservoirKernelTest, Test****) for each function in a class. More than one function should be used to test different asusmptions/code paths. i.e. TEST_F(ReservoirKernelTest, TestReservoirAddOutlet_1) might test that all assertions are good when reservoirs are added in an already sorted order. TEST_F(ReservoirKernelTest, TestReservoirAddOutlet_2) might test that the outlet states are all valid when an outlet with too large of activation is added...

We might want to refactor the testing at some point so we have something like
TEST_F(ReservoirKernelTest, TestReservoirAddOutlet)
{
NoOutletReservoir2->add_outlet(ReservoirLinearOutlet);
NoOutletReservoir2->add_outlet(0.3, 0.5, 0.0, 100.0);
ASSERT_TRUE(true);
}

@jdmattern-noaa will address.

We currently have the data needed to run the driver packed into the repo. As of this writing, if you clone the repo and try to run it, it should be able to run. Unfortunately, some of the sample data was moved, invalidating some of the paths in the driver. It's a small, easy change, but it would have been nice if a message was created on the PR generation stating "Hey, this will error out".

It will probably be helpful to run this in the testing action and assert that the exit code was correct in order to make sure it was able to run until completion.

Here is an example where a waterbody ID is being treated as a catchment id in order to associate a COMID with a catchment to read its GIUH data. This is technically the wrong semantics, and should not propagate further.

One potential solution is to add the catchment id to the crosswalk.json file. This has been raised on hygeo as a potential fix.

dblodgett-usgs/hygeo#15

This slipped by in review, but the index pointer here doesn't need to be a literal pointer, just the int index that keeps track of where in the forcing vector the object is looking at the moment.

Just make this an int on the stack int forcing_vector_index_ptr; initialize to 0 in constructor, increment as designed (minus the pointer deference).

Opening this issue to draw attention to the PR template. The template needs better alignment with the project; the template copied at repo init don't appropriately capture the nature of this work. Specific attention needed in the PR checklist, Accessibility, and Other sections.

To be consistent with variable naming conventions, all variables should be lowercase

@jdmattern-noaa Per Fred's comments, will need to add a max flow parameter to these outlets.

All the variables in this model kernel need to be renamed and documented to be clear, meaningful names with units.

This file was committed (I believe) for its infil() function. If this is the kernel functionality we want, we need to remove main(). I would recommend moving the test code in main into a unit test.

It would be beneficial as a user and potential contributor if a link to the project specific GitHub Pages were linked in the About or root level readme. I would be happy to open a PR adding this to the readme if you all see fit.

Short description explaining the high-level reason for the new issue.

Current behavior

Tests run but fail

[----------] 1 test from ForcingTest
80
[ RUN ] ForcingTest.TestForcingDataRead
81
/home/Z/actions-runner/_work/_temp/1ac2add5-d50d-471a-994c-51763b638d59.sh: line 1: 5313 Segmentation fault (core dumped) ./cmake_build/test/test_all
82
##[error]Process completed with exit code 139.

Expected behavior

Tests should pass

Steps to replicate behavior (include URLs)

https://github.com/ZacharyWills/ngen/runs/671741082

Screenshots

Support for dynamic hydrofabric realization construction.
Relative to (tentative) sprint 5 task 4.

Current behavior

Hydrofabric definitions are statically linked to a single input file/representation.

Expected behavior

A realization is defined by a type in realization_config.json input. Realizations will have to be connected to parameters, either in this file or in another file linked by identity.

One parameter of a realization could be its hydro-fabric definition, so when the driver factory creates a realization, it passes the realization the location of its hydro-fabric allowing each realization to be instantiated from from potentially different input.

Simple use case: two realizations reading the same hydro-fabric defined in two different independent input files.

The response_meters_per_second function in the Reservoir class does not handle calculating excess water properly in all cases.

Current behavior

• In some cases, the function does not initialize the excess_water_meters parameter (passed in as a reference and used to indirectly return the excess amount), which it appears to be designed to do, even if the value should be 0.0.
• In cases when there is excess - i.e., when the reservoir's state.current_storage_height_meters is greater than parameters.maximum_storage_meters - the function calculates the excess amount after resetting state.current_storage_height_meters to be equal to the max, which will thus always make the excess value 0.0.

Expected behavior

The pass excess reference should always be initialized to either 0.0 or the amount of the reservoir's current storage that is greater than its max capacity, at the end of of each call to response_meters_per_second.

This function must bound check the index at minimum to prevent seg faults. Might also be worth implementing an unordered map "name" outlets. i.e. unoderedmap<string, int> outlet_map then outlet_map[name] = index. Could then lookup by name outlet_map['lateral_flow'] so this function could take a string name arg.

Current behavior

Compiler version: g++ (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39)
CMake version: cmake3 version 3.14.6

Bug not present for other users with previous compiler versions.

Errors from:
ngen/test/geojson/JSONGeometry_Test.cpp
ngen/test/geojson/Feature_Test.cpp

Multiple instances of this line in both files cause compilation errors:
stream = std::stringstream();

Compiles fine when corrected to:
stream.str("");

Expected behavior

PR to follow with this fix.

Steps to replicate behavior (include URLs)

1. Clone repo: git clone https://github.com/NOAA-OWP/ngen.git
2. cd ngen
3. Update googletest: git submodule update --init --recursive -- test/googletest
4. Create build directory: cmake -DCMAKE_BUILD_TYPE=Debug -B cmake-build-debug -S
5. Compile with CMake: cmake --build cmake-build-debug --target test_unit -- -j 4

The Nexus read/inspection API can be used to couple FIM/Waterbody models to FlowPaths to provide information to a waterbody.

Need to differentiate Realization stuff from Formulation stuff in the NGEN class hierarchy, and build an explicit HY_FlowPath realization -> formulation middle ware.

This will help connect hydrologic dependencies in a consistent, automatic way.

We may need a hybrid realization type which does area stuff and flow path stuff, i.e. is aware of hydrologic channel routing properties and connected explicitly with hydrologic land surface components.

This may also require implementation of some/all of the HY_HydroLocation concepts.

@dblodgett-usgs @christophertubbs @BrianAvant-NOAA any additional thoughts or comments are welcome!

Related to #29 and #41. Need a more descriptive public interface function from pdm03, an the calling wrapper here should reflect a similar, if not verbatim, name.

The only way to link features to their ids and neighbors is to parse the locations, iterate through them and read the ids from their proper attributes or properties, set the id to that value, then call the FeatureCollection function to link everything. If you're lucky and have a geojson with regular id member values, a third of the work may be done during parsing. You'll still need to call functions afterwards.

Instead, reading and linking everything should be more natural, requiring only one or two functions to parse, ID, and link everything.

When reading from the gh-pages repo for the github page, changes need to be in the root instead of docs or docs/html. As a result, proper, new information is not in place to be read.

Since NGen is being initially targeted for unix machines, developing on windows can be troublesome. A workaround for this is gitpod. Gitpod isn't perfect, but can provide a space to develop, build, and run NGen from a convenient location.

Instructions for how to get started should be added to the contributing documentation and possibly the readme.

In PR #27 a new flux/physics kernel module was added, Pdm03.h
This kernel needs to be updated to comply with naming conventions and the file itself should get a more descriptive name.

Current behavior

Pdm03.h is not up to standard for naming conventions.

Expected behavior

More clear file, interface, and variable names.

Current behavior

shared_ptr is not in std namespace on Linux based systems and will therefore cause compilation error.

Proposed

Need to add #include <memory> and will compile.

In order for the github pages to work, a _config.yml must be provided. It's in the master branch, but not gh-pages.

Without it, I believe that it tries to read the source documents from an incorrect directory, preventing data from being properly deployed and resulting in a 404.

Need to review comments on #147 and open issues to address some of the refactoring and cleanup that was mentioned in that PR. The PR was merged to allow other work to continue in development, but some of the noted changes need to be addressed.

Initial code included the header, not the implementation of the point nexus.

Builds fail with versions of boost < 1.72.

Current behavior

1.72 is only recommended, but builds fail with 1.69

Expected behavior

Minimum version should be enforced in cmake as 1.72 and explicitly required in dependencies.md

The current process for creating a set of realizations from the configuration involves reading the JSON, loading specifications into Realization_Config objects, then later calling get_realization() from the config object, which then calls a separate get_X function for the proper realization type, which then reads a static array of required fields, then constructs the parameters and calls the constructor for the appropriate realization.

Instead, the JSON reading should yield the JSON objects that can then be used to create the realizations and bypass Realization_Config altogether.

The arrays of parameter names can then be moved to the realizations themselves and each realization can have a constructor that takes the JSONProperties as a parameter.

Current behavior

Automated testing currently builds and runs on ubuntu-latest and macos-latest environments.

Expected behavior

Need to be about to test some customized tool chains, in particular a centos/rhel 7 environment using gcc 4.8.5.

Propose using a docker container action (which can run on the github hosted ubuntu vm) to define some custom test environments.

https://help.github.com/en/actions/creating-actions/creating-a-docker-container-action

The current calc_soil_field_capacity_storage function in the tshirt_model class does not use the exact calculation as described in our documentation for the Tshirt model. Additionally, the function is for calculating the field capacity storage threshold, which could potentially be ambiguous or unclear based on the current name.

Forcing.h gets repeated warnings on build because one of the constructors sets basin_id(basin_id), but, since no basin_id is passed, the constructor just sets the basin_id as the value it already was (which wasn't anything).

Would anyone be opposed to me setting that as 0 to ensure that it's at least getting the default value or just taking it out of the signature?

HYMOD changes should be described here:
https://github.com/NOAA-OWP/ngen/blob/master/models/hymod/include/Hymod.h

Current behavior

I copied the HYMOD README.md from the repository but we changed some things in our implementation. These should be reflected by changing the README.md from the original to reflect any changes.

Expected behavior

HYMOD should have a README that describes our version used for ngen

Steps to replicate behavior (include URLs)

Screenshots

Request to simplify GIUH output file.

Current behavior

The GIUH.json file contains attributes that may not be used in ngen and removing them could greatly reduce file size. Current attributes include cumulative frequency distribution ('CumulativeFreq'), incremental runoff per hour ('hrHydro') and incremental runoff per minute ('minHydro') time series. It looks like only 'CumulativeFreq' is being used in the demo. Do we need these other attributes? We should be able to derive incremental runoff for any time step from the cumulative frequency time series. @hellkite500 @robertbartel

Expected behavior

Remove redundant attributes from GIUH.json

Steps to replicate behavior (include URLs)

Screenshots

This function will segfault if an index is passed that is not in range 0 -> features.size().

The infiltration interface should have descriptive parameter names with units and appropriate documentation.

These two kernels have duplicate code for date/time handling, this needs to be refactored.

The code in model/kernel/utility.hpp is legacy code used to support the testing of some translated kernels. This has memory allocation using malloc() which should be replaced with more c++ centric memory management, i.e. vectors for arrays of double/float.

I have GCC 4.8.5. I get this error at build:

ngen/src/NGen.cpp:229:33: error: use of deleted function ‘std::basic_ofstream& std::basic_ofstream::operator=(const std::basic_ofstream&)’
nexus_outfiles[feat_id] = std::ofstream("./"+feature->get_id()+"_output.csv", std::ios::trunc);

Expected behavior

My build worked fine with Donald's recent PR before pulling the latest updates from June 30 and July 1.

Each geojson class has a function called geometry() that returns their respective geometry from within its variant. It's not implemented on its parent class, however. As a result, if you try to call geometry() on a FeatureBase instance (not a properly case PolygonFeature, PointFeature, etc), you get an error on compilation because it can't find the symbol.

Need to have HY_HydroNexus as a purely topological entity that manages neighboring relationship/identities.

NexusRealizations use these relationships, implementing also HY_HydroLocation. Specific transfer of fluxes/information become formulations attributed to these classes.

@donaldwj @christophertubbs @robertbartel @dblodgett-usgs

The usage of the Reservoir::response_meters_per_second function in the tshirt_model class is not entirely correct/consistent, especially regarding parameters obtained from Schaake partitioning. These need to be corrected.

More generally, it should be considered whether the function's parameter for how much new water is being introduced should be a depth-type value in the form of meters (thereby implicitly meters per timestep), or a volumetric value with units of meters per second (as is currently the case).

RHEL/CentOS 7 gcc tool chain is 4.8.5 and even with --std=c++1y (for c++14) make_unique is not supported.

Current behavior

Build fails using gcc 4.8.5 with c++14 standards

Expected behavior

Buildable on RHEL/ContOS 7

##Proposed
add a custom make_unique<>() implementation in utilities with a macro gaurd to enable it for gcc < 4.9