-
Notifications
You must be signed in to change notification settings - Fork 17
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
Allow Modeling Two Speed & Var Speed Geothermal Heat Pumps #1878
base: master
Are you sure you want to change the base?
Conversation
HPXMLtoOpenStudio/resources/hvac.rb
Outdated
# Recoverable heat modifier as a function of indoor wet-bulb and water entering temperatures. | ||
waste_heat_ft = Model.add_curve_biquadratic( | ||
model, | ||
name: "WastHeat-FT#{i + 1}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"WasteHeat" (I think there's a typo, missing the "e")
…o ghp-two-speed-var-speed
Update: Performance curve coefficients for two speed systems are completed. |
…o ghp-two-speed-var-speed
…o ghp-two-speed-var-speed
coeff: [1, 0, 0], | ||
min_x: 0, max_x: 1, min_y: 0.7, max_y: 1 | ||
) | ||
clg_coil = OpenStudio::Model::CoilCoolingWaterToAirHeatPumpVariableSpeedEquationFit.new(model, plf_fplr_curve) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be sure to audit the code for references to e.g. CoilCoolingWaterToAirHeatPumpEquationFit
. I'm seeing at least two places in our code (installation quality EMS program and desuperheaters) that presumably need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about desuperheaters, the variable speed coil type is not yet supported by E+ to be connected with desuperheater, so we may have to temporarily ignore that feature (and document that only single speed gshp is currently supported).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shorowit @afontani The E+ IO freeze for March release is 2/12, is it still possible to add this capability to Coil:Cooling:WaterToAirHeatPump:VariableSpeedEquationFit for the upcoming release? There're 12 open PR for the E+ team to review before 2/12, so I'm not sure if it's possible to add this one and get someone's review. Also the date is overlapping with the two speed/var speed tasks (plan to be wrapped up before end of Feb). Is it fine if we prioritize the two/var speed system work, then if we still get some space we can add the capability to the E+ Oct release?
We can explicitly document that the 2/var speed is not currently supporting desuperheater, and if user still needs to model it, the workaround will be specifying a single speed system (instead of defaulting based on efficiency, or specifying it as two/var speed systems) with desuperheater, which will be handled in the same way as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it should be easy to review and there are many possible reviewers, I wouldn't worry about that.
As far as I'm aware, you are allowed to just make the IDD change before the IO freeze and complete the implementation later. So that could give you another 4-6 weeks? You could confirm by asking Edwin or Mike.
Documenting that we are introducing a breaking change is not a great solution. For a downstream software tool (like REM), it could be a lot of work -- adding a new compressor type input in their user interface for GSHPs, adding logic when there's a desuperheater, etc. And their user would get different heating/cooling results based on whether there is a desuperheater or not? That's confusing.
In my opinion, either we make it a non-breaking change or we defer this for a subsequent OS-HPXML release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it should be easy to review and there are many possible reviewers, I wouldn't worry about that.
As far as I'm aware, you are allowed to just make the IDD change before the IO freeze and complete the implementation later. So that could give you another 4-6 weeks? You could confirm by asking Edwin or Mike.
Documenting that we are introducing a breaking change is not a great solution. For a downstream software tool (like REM), it could be a lot of work -- adding a new compressor type input in their user interface for GSHPs, adding logic when there's a desuperheater, etc. And their user would get different heating/cooling results based on whether there is a desuperheater or not? That's confusing.
In my opinion, either we make it a non-breaking change or we defer this for a subsequent OS-HPXML release.
The IDD change before IO freeze and implement later sounds interesting, I'm fine with that, assuming we'll wrap up most of the 2/var speed system OS-HPXML changes before that. Do you mean that it's better to wait for the E+ release and this PR is non-breaking then we can merge it? @afontani Is it fine to do so? The E+ release is end of March, so probably the merge will be early April, but we can still have most functions being implemented so that ResStock can test the batch runs based on this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another thought to make it non-breaking to merge early. So what about if we exclude the efficiency-based defaulting for now, and always default the compressor type to be single speed, and when the E+ capability is added then we add the efficiency-based defaulting approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like a great idea/compromise to me!
…ed conditions, added more inputs, a few questions, store progress
@@ -2807,23 +2807,37 @@ def self.apply_hvac_equipment_adjustments(mj, runner, hvac_sizings, weather, hva | |||
|
|||
gshp_coil_bf = 0.0806 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can look at E+ bypass factor calculations.
…o ghp-two-speed-var-speed
…low quadratic curves (fixed coefficients orders, and fixed some curves that are not normalized).
…o ghp-two-speed-var-speed
…o ghp-two-speed-var-speed
…o ghp-two-speed-var-speed
…o ghp-two-speed-var-speed
Pull Request Description
Allow optional compressor type inputs to be specified. Allow modeling two and variable speed geothermal heat pumps.
Checklist
Not all may apply:
EPvalidator.xml
) has been updatedopenstudio tasks.rb update_hpxmls
)HPXMLtoOpenStudio/tests/test*.rb
and/orworkflow/tests/test*.rb
)openstudio tasks.rb update_measures
has been run