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

Fix (Probable) Fan Coil Sizing Issue #10461

Merged
merged 3 commits into from
Apr 9, 2024
Merged

Fix (Probable) Fan Coil Sizing Issue #10461

merged 3 commits into from
Apr 9, 2024

Conversation

amirroth
Copy link
Collaborator

@amirroth amirroth commented Apr 8, 2024

This is a very quick one. In the process of testing a set of refactoring changes I ran into a unit test failure where the fan index was not set on an FCU until after sizing, causing the heat generated by the fan to not be accounted. It turns out that this diff was caused by a line that was commented out, likely in error. A comment above the line confirms this suspicion. @rraustad confirms it also. The only change in this PR is to uncomment this line.

@Myoldmopar Myoldmopar added the Defect Includes code to repair a defect in EnergyPlus label Apr 8, 2024
@amirroth amirroth requested review from Myoldmopar and rraustad April 9, 2024 00:09
@amirroth amirroth added this to the EnergyPlus 24.2 milestone Apr 9, 2024
EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerSecNode).MassFlowRate, 0.369714, 0.000001);
EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerOutNode).MassFlowRate, 0.569714, 0.000001);
EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerSecNode).MassFlowRate, 0.350865, 0.000001); // Was 0.369714
EXPECT_NEAR(state->dataLoopNodes->Node(thisFanCoil.ATMixerOutNode).MassFlowRate, 0.550865, 0.000001); // Was 0.569714
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is the same thing I see. I still can't figure out why this unit test fails now and not last year when that 1 line (line 515 in FanCoilUnits) was commented out (#9932 on Apr 3, 2023). It makes sense that the cooling coil is now larger (by 496.3 W of fan power) and the PLR is lower which changes the air flow numbers. The example file FanCoilAutoSize skips sizing the first time through but calls the fan (also skips sizing), which sets the fanIndex so there is no change in results when sizing finally does occur. That explains the example files but not this unit test.

@rraustad
Copy link
Contributor

rraustad commented Apr 9, 2024

@Myoldmopar I tried to merge this and came up with a failed attempt.

Copy link
Contributor

@rraustad rraustad left a comment

Choose a reason for hiding this comment

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

OK to merge

@Myoldmopar Myoldmopar merged commit 75c995a into develop Apr 9, 2024
13 of 14 checks passed
@Myoldmopar Myoldmopar deleted the FanCoilSizingIssue branch April 9, 2024 13:25
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.

7 participants