Multi level hydrofabric #615
Conversation
include/core/Layer.hpp
Outdated
| //m/timestep | ||
| //since we are operating on a 1 hour (3600s) dt, we need to scale the output appropriately | ||
| //so no response is m^2/hr...m^2/hr * 1hr/3600s = m^3/hr | ||
| response /= 3600.0; |
There was a problem hiding this comment.
So, there was no great place to have a timestep variable before... but now we have a Simulation_Time object with an output_interval... even if we assume that the output is m/h (not m/timestep!), shouldn't this response be divided by the number of timesteps per hour if the timestep is not = 1?
There was a problem hiding this comment.
this was fixed in my branch
include/core/Layer.hpp
Outdated
| auto r = features.catchment_at(id); | ||
| //TODO redesign to avoid this cast | ||
| auto r_c = std::dynamic_pointer_cast<realization::Catchment_Formulation>(r); | ||
| double response = r_c->get_response(output_time_index, description.time_step); |
There was a problem hiding this comment.
will description.time_step and simulation_time.get_output_interval_seconds() ever differ? Are they redundant or is there a current/potential future semantic difference?
There was a problem hiding this comment.
Actually since it looks like description has a time_step_units, it's likely that description.time_step should not be used here and simulation_time.get_output_interval_seconds() should be used instead, or that description.time_step should be put through a unit conversion first (probably the former).
mattw-nws
force-pushed
the
multi_level_hydrofabric
branch
from
8aa1277 to
7a0d40a
Compare
mattw-nws
force-pushed
the
multi_level_hydrofabric
branch
2 times, most recently
from
34e95c0 to
fc67105
Compare
.gitignore
Outdated
| @@ -97,7 +97,6 @@ CMakeCache.txt | |||
| /cmake-build-debug | |||
| /cmake_build | |||
| /build | |||
| /cmake_build* | |||
There was a problem hiding this comment.
PR should leave .gitignore alone - it's out of scope.
| // layers with small time steps to be updated more than once | ||
| // If a layer with a large time step is after a layer with a small time step the | ||
| // layer with the large time step will wait for multiple timesteps from the preceeding | ||
| // layer. |
There was a problem hiding this comment.
Since we're not (currently) doing temporal interpolation, should we be asserting that the various time step increments are all (close enough to) integer ratios, so that we're never left with a layer/model at an unaligned time?
There was a problem hiding this comment.
Interesting... maybe, but not necessarily...uninterpolated sampling is still a method of sampling. If the value is a rate (which we highly encourage and I think the formulations team is now favoring and moving their code to as a rule) then this doensn't matter much ...for most common cases. Rapid fluctuations or large timestep differences could lead to significant differences in results, but I don't know if we'd want to fail if they don't line up in many simpler "okay" cases. A warning might be worth considering.
|
@donaldwj Do you have any uncommitted/unpushed updates to this PR? I'm going to rebase on master again to make sure things work and clean up a few of the minor issues. |
…atchment level. Added constant variables to mark predefined catchment level values.
…here level was not a string.
improve readability
mattw-nws
force-pushed
the
multi_level_hydrofabric
branch
from
110de1d to
68a53bf
Compare
set of levels is now correctly updated.
…at works with levels.
Co-authored-by: Phil Miller - NOAA <[email protected]>
…ion time object. Conflicts: include/core/Layer.hpp
Co-authored-by: Phil Miller - NOAA <[email protected]>
mattw-nws
force-pushed
the
multi_level_hydrofabric
branch
from
68a53bf to
f4df6d2
Compare
src/core/HY_Features.cpp
Outdated
|
|
||
| // get the catchment level from the hydro fabric | ||
| const auto& cat_json_node = fabric->get_feature(feat_id); | ||
| long lv = cat_json_node->has_key("level") ? cat_json_node->get_property("level").as_natural_number() : 0; |
There was a problem hiding this comment.
I was going through and unifying "level" to "layer" for consistency... but this appears it would actually have a functional impact, because the provided /data/catchment_data_multilayer.geojson file actually does have "layer" keys instead of "level"... of course the test code was previously looking for a different example file which was not committed, so I don't know what was in that one. @donaldwj can you look at this please? I will test some with this but am not as confident I'll figure out the impact.
There was a problem hiding this comment.
After changing this, it also passes the included test with the example file that has "layer"... for better and worse.
src/core/HY_Features_MPI.cpp
Outdated
|
|
||
| // get the catchment level from the hydro fabric | ||
| const auto& cat_json_node = linked_hydro_fabric->get_feature(feat_id); | ||
| long lv = cat_json_node->has_key("level") ? cat_json_node->get_property("level").as_natural_number() : 0; |
There was a problem hiding this comment.
Ditto for comment in non-MPI version of this file, @donaldwj
src/core/network.cpp
Outdated
| { | ||
| if ( feature->has_property("level") ) | ||
| { | ||
| const auto& prop = feature->get_property("level"); |
There was a problem hiding this comment.
This again doesn't match the data/catchment_data_multilayer.geojson file @donaldwj
[Short description explaining the high-level reason for the pull request]
Additions
Removals
Changes
Testing
Manual Test of 5 catchment data set
Screenshots
Notes
Todos
Checklist
Testing checklist (automated report can be put here)
Target Environment support