Simplify sys params to closer match reality #659
Conversation
There was a problem hiding this comment.
Hi @vtnate , please see the detailed review comments. Otherwise, it looks good!
| @@ -118,7 +116,8 @@ | |||
| "chp_installed": false | |||
| }, | |||
| "central_pump_parameters": { | |||
| "pump_design_head": 60000 | |||
| "pump_design_head": 60000, | |||
| "pump_flow_rate": 0.01 | |||
There was a problem hiding this comment.
I think for fourth gen districts, the central pump design head and flow rate should be under central cooling/heating plant parameters, as in chw_pump_head, cw_pump_head, and chiller_water_flow_nominal in the cooling plant, and pressure_drop_hhw_nominal and mass_hhw_flow_nominal in the heating plant.
There was a problem hiding this comment.
In other words, these should be 5G only changes.
There was a problem hiding this comment.
Should the entire central_pump_parameters block be removed from 4G districts?
We still want to remove the head & flow-rate from all ETS parameters, correct (as I did in this PR)?
There was a problem hiding this comment.
To your first question, yes. Can you provide an example of your second question?
There was a problem hiding this comment.
I mean deleting the lines from the building ETS parameters. Confirming that what I did in this PR is appropriate.
There was a problem hiding this comment.
Yes, deleting the pump_flow_rate and pump_design_head from the 5g ETS parameters is appropriate.
| }, | ||
| "central_pump_parameters": { | ||
| "pump_design_head": 60000, | ||
| "pump_flow_rate": 0.01 |
There was a problem hiding this comment.
ThermalNetwork should be aware of these changes for its sizing of GHEs.
There was a problem hiding this comment.
I have an open PR on TN doing just that: NREL/ThermalNetwork#37.
| @@ -217,7 +217,7 @@ | |||
| "type": "object", | |||
| "properties": { | |||
| "geojson_id": { | |||
| "description": "The GeoJSON ID as defined in the GeoJSON file. This is used to overwrite the default data for a specific building. This option is not used in the default section.", | |||
| "description": "The GeoJSON ID as defined in the GeoJSON file.", | |||
Any background context you want to provide?
We developed the schema to cover many potential 5G parameters. This cleans up a few of them, to reduce duplication and have parameters where they make sense.
This says 154 commits but that is because it was originally based off the multi-ghe PR, so Github thinks those 140 commits are a part of this PR. It's not that big, just changing schema and test sys-param files to match.
What does this PR accomplish?
pump_design_head&pump_flow_rateto district params instead of individual ETS params.How should this be manually tested?
CI is technically efficient. Manual review to determine if this is the appropriate place for them to be.
What are the relevant tickets?
Resolves #629