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

Addressed FanCoil with CyclingFan control using Fan:SystemModel has different fan power than Fan:OnOff #7380

Merged
merged 2 commits into from
Aug 26, 2019

Conversation

Nigusse
Copy link
Contributor

@Nigusse Nigusse commented Jul 9, 2019

Pull request overview

Fixes #7086. Fixed performance differences between a fan coil unit with cycling fan using Fan:OnOff and Fan:SystemMode. Now, fancoil units with cycling fan operating mode, will give the same result using either Fan:OnOff or Fan:SystemMode. Diffs are expected when a FanCoil unit uses Fan:SystemModel fan type and the capacity control method is CyclingFan.

Work Checklist

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

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description

Review Checklist

This will not be exhaustively relevant to every PR.

  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things

@Nigusse Nigusse added the Defect Includes code to repair a defect in EnergyPlus label Jul 9, 2019
@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 9, 2019

Diffs are expected in the example file "HVACTemplate-5ZoneFanCoil-DOAS.idf", because the FanCoil units has Fan:SystemModel fan type and the capacity control method is CyclingFan.

@mjwitte mjwitte added this to the EnergyPlus 9.2.0 milestone Aug 5, 2019
@Myoldmopar
Copy link
Member

Yes, diffs are indeed expected and acceptable. Testing locally now.

@@ -3594,8 +3594,7 @@ namespace FanCoilUnits {
ZoneCompTurnFansOn,
ZoneCompTurnFansOff);
} else {
HVACFan::fanObjs[FanCoil(FanCoilNum).FanIndex]->simulate(
FanCoil(FanCoilNum).LowSpeedRatio, ZoneCompTurnFansOn, ZoneCompTurnFansOff, _);
HVACFan::fanObjs[FanCoil(FanCoilNum).FanIndex]->simulate(_, ZoneCompTurnFansOn, ZoneCompTurnFansOff, _);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you walk me through this change? By not passing a speed ratio, does this force the fan object to operate at a single discrete mode? Is that what fixes the fan to operate just like the Fan:OnOff?

Copy link
Contributor Author

@Nigusse Nigusse Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to pass the fan speed ratio for cycling operating mode. FanCoilUnits cycling mode are designed such that the fan coil unit cycles at each speed level. Thus passing speed ratio will result incorrect fan power calculation. By not passing the fan speed ratio, the fansystem model locally calculates average flow fraction instead, for the fan power calculation.

Fans::SimulateFanComponents(
FanCoil(FanCoilNum).FanName, FirstHVACIteration, FanCoil(FanCoilNum).FanIndex, 0.0, ZoneCompTurnFansOn, ZoneCompTurnFansOff);
} else {
HVACFan::fanObjs[FanCoil(FanCoilNum).FanIndex]->simulate(0.0, ZoneCompTurnFansOn, ZoneCompTurnFansOff, _);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change required for fixing this defect or is this an unrelated change?

Copy link
Contributor Author

@Nigusse Nigusse Aug 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you referring to the last line where I set 0.0 for speed ratio, is to turn off the fan. Simple because the unit is off if no fan speed is specified.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fancoil unit is expected to operate at one of the three fan speed levels, if the fan speed is not set yet, then the fan coil unit must be off. Otherwise, what does the fancoil unit do if no fan speed set yet? I observed such a case when I was debugging the defect.

EXPECT_NEAR(FanCoil(1).PLR, 0.753, 0.001);
EXPECT_NEAR(QZnReq, QUnitOut, 5.0);
EXPECT_NEAR(Node(1).MassFlowRate, thisFanCoil.PLR * thisFanCoil.MaxAirMassFlow * thisFanCoil.MedSpeedRatio, 0.0000000001);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not asking you to make a change to the unit test right now but I will say it would be nice to set up a unit test with both fan onoff and system model connected to identical but separate systems. Then you could simulate both of them and check that the values are equal. This would avoid having to hardwire expected values in the unit test. Just a note for future tests like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. @Myoldmopar Thanks.

@Myoldmopar
Copy link
Member

I just ran the OnOff and the SystemModel defect files with both develop and this branch. I see the big difference in results in the develop branch between the two files. With this branch I see the results come together for the two fan types. I think this is ready to go in, but I'd still like a clarification of the changes on my review above so I fully understand it and so they are documented here for the future.

Thanks @Nigusse .

@Myoldmopar
Copy link
Member

(Also, I just ran unit test suite locally from this branch with develop merged in and it ran fine.)

@Myoldmopar
Copy link
Member

Thanks for the clarifications @Nigusse. Merging now.

@Myoldmopar Myoldmopar merged commit 1992e56 into develop Aug 26, 2019
@Myoldmopar Myoldmopar deleted the 167166482_Issue7086 branch August 26, 2019 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FanCoil with CyclingFan control using Fan:SystemModel has different fan power than Fan:OnOff
8 participants