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

Add Fan:SystemModel to OpenStudio #3856

Merged
merged 61 commits into from
Jan 22, 2020
Merged

Add Fan:SystemModel to OpenStudio #3856

merged 61 commits into from
Jan 22, 2020

Conversation

jmarrec
Copy link
Collaborator

@jmarrec jmarrec commented Jan 21, 2020

Pull request overview

NOTES

  • I have removed the "Number of Speeds" field from the OpenStudio.idd altogether, the number of extensible groups is enough.
  • Technically, when Speed Control Method is "Discrete", you can add Speeds (extensible) by specifying only the Speed Flow Fraction and omitting the Speed Electric Power Fraction if you have used an Electric Power Function of Flow Fraction Curve (power fraction is then obtained by evaluating the curve at the given Flow Fraction specified). I have disabled this behavior. Speeds can only be added by specifying both flow fraction and power fraction. (Well, in fact you can still manually use pushExtensibleGroup then use setDouble to only set the flow Fraction, I didn't make Speed Electric Power Fraction a required field..)
  • I am NOT providing extensive logic in the FT such as checking that if a ZoneHVACFourPipeFanCoil is "VARIABLEFANVARIABLEFLOW" and using a FanSystemModel, then the FanSystemModel needs to have Speed Control = Continuous. There are probably too many rules to implement correctly, so I'm letting E+ throw fatals for that.
  • There are a bunch of objects that should accept Fan:SystemModel but actually don't due to IDD \object-list and \references not matching. In particular, Fan:SystemModel isn't part of FanOnOff while a bunch of ZoneHVAC objects accept Type= Fan:SystemModel but Name only FanOnOff. Cf Fan:SystemModel inconsistencies in IDD EnergyPlus#7697
    • Not sure if I should modify our OpenStudio.idd and ProposedEnergy+.idd or wait for it to be implemented in EnergyPlus to begin with.

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Model API Changes / Additions
  • Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
  • Model API methods are tested (in src/model/test)
  • EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
  • If a new object or method, added a test in NREL/OpenStudio-resources: Add test for newly added Fan:SystemModel OpenStudio-resources#92
  • If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp): N/A
  • Checked behavior in OpenStudioApplication, adjusted policies as needed (src/openstudio_lib/library/OpenStudioPolicy.xml): Add Fan:SystemModel to OpenStudioApplication openstudiocoalition/OpenStudioApplication#72
  • Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
  • All new and existing tests passes
  • If methods have been deprecated, update rest of code to use the new methods: N/A

Labels:

  • If change to an IDD file, add the label IDDChange
  • If breaking existing API, add the label APIChange
  • If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • Code Style, strip trailing whitespace, etc.
  • All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
  • Labeling is ok
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified

…t in Ctor (see details below:)

```
setAvailabilitySchedule(model.alwaysOnDiscreteSchedule());
autosizeDesignMaximumAirFlowRate();
setDesignPressureRise("????");
setSpeedControlMethod("Discrete");
setElectricPowerMinimumFlowRateFraction(0.2);
setMotorEfficiency(0.9);
setMotorInAirStreamFraction(1.0);
autosizeDesignElectricPowerConsumption();

setElectricPowerPerUnitFlowRatePerUnitPressure(1.66667);
=> setDesignPowerSizingMethod("PowerPerFlowPerPressure");

setFanTotalEfficiency(0.7);
setNumberofSpeeds(1);
```
Probably needs cleanup, deal with output directory, etc
…d (this one has a default), and the Electric Power Per Unit Flow Rate too (this one I'll create a default)
…e are going to fail, like AirLoopHVACUNitaryHeatPumpxx that doesn't support it), clone,
@jmarrec jmarrec added component - HVAC component - Model Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Jan 21, 2020
@jmarrec jmarrec requested a review from kbenne January 21, 2020 15:06
@jmarrec jmarrec self-assigned this Jan 21, 2020
@kbenne
Copy link
Contributor

kbenne commented Jan 21, 2020

NOTES

  • I have removed the "Number of Speeds" field from the OpenStudio.idd altogether, the number of extensible groups is enough.

KB: Strongly agree with your decision. I also try to reduce this kind of redundancy when I see it.

  • Technically, when Speed Control Method is "Discrete", you can add Speeds (extensible) by specifying only the Speed Flow Fraction and omitting the Speed Electric Power Fraction if you have used an Electric Power Function of Flow Fraction Curve (power fraction is then obtained by evaluating the curve at the given Flow Fraction specified). I have disabled this behavior. Speeds can only be added by specifying both flow fraction and power fraction. (Well, in fact you can still manually use pushExtensibleGroup then use setDouble to only set the flow Fraction, I didn't make Speed Electric Power Fraction a required field..)

KB: This seems ok to me. In general I like not needing to have a decision tree diagram to figure out what is required input data. Does this choice avoid any optional fields in the API? If so then I am doubly in favor.

  • I am NOT providing extensive logic in the FT such as checking that if a ZoneHVACFourPipeFanCoil is "VARIABLEFANVARIABLEFLOW" and using a FanSystemModel, then the FanSystemModel needs to have Speed Control = Continuous. There are probably too many rules to implement correctly, so I'm letting E+ throw fatals for that.

KB: Yeah and if we do try to implement rules within OpenStudio we will probably fail in keeping them in sync with EnergyPlus over time. Also if you were to implement these rules in the model API (as opposed to the FT) how would you communicate the dynamically required/optional fields as related field change? I think your choice is the correct one. Let EnergyPlus report any input errors in this case.

  • There are a bunch of objects that should accept Fan:SystemModel but actually don't due to IDD \object-list and \references not matching. In particular, Fan:SystemModel isn't part of FanOnOff while a bunch of ZoneHVAC objects accept Type= Fan:SystemModel but Name only FanOnOff. Cf NREL/EnergyPlus#7697

KB: My first reaction is follow the EnergyPlus IDD even if it is wrong. Consider a PR to EnergyPlus instead, and then update OpenStudio IDD if EnergyPlus merges. Don't you think that even if you correct the OpenStudio IDD you might still have a failed simulation when EnergyPlus can't validate against its IDD?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component - HVAC component - Model IDDChange Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Fan:SystemModel to OpenStudio
2 participants