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 space sizing output (spsz) when there is no space HVAC equipment #10947

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

mjwitte
Copy link
Contributor

@mjwitte mjwitte commented Feb 20, 2025

Pull request overview

Description of the purpose of this PR

  • Write the spsz output file anytime "Do Space Heat Balance for Sizing" is Yes. Previously it was only written if the idf also included SpaceHVAC:* objects.
  • Avoid opening and leaving an empty spsz output file if it will not be filled.

Pull Request Author

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • 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
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs
    `

@mjwitte mjwitte added Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Feb 20, 2025
@mjwitte mjwitte added this to the EnergyPlus 25.1 IO Freeze milestone Feb 20, 2025
@mjwitte mjwitte marked this pull request as ready for review February 21, 2025 03:43
@@ -4301,4 +4301,6 @@ TEST_F(EnergyPlusFixture, SizingManager_ZoneSizing_Coincident_NonAir_10x_Latent_
EXPECT_EQ("CHICAGO_IL_USA ANNUAL COOLING 1% DESIGN CONDITIONS DB/MCWB",
OutputReportPredefined::RetrievePreDefTableEntry(*state, state->dataOutRptPredefined->pdchZnClDesDay, "ZONE 1"));
EXPECT_EQ("7/21 16:00:00", OutputReportPredefined::RetrievePreDefTableEntry(*state, state->dataOutRptPredefined->pdchZnClPkTime, "ZONE 1"));
// Expect output to spsz file
EXPECT_FALSE(state->files.spsz.get_output().empty());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't work. Tried several different ways to check the contents of this file.

@mjwitte
Copy link
Contributor Author

mjwitte commented Feb 21, 2025

Tested with example files:
5ZoneAirCooledWithSpaces - no spsz output before and after fix
5ZoneAirCooledWithSpaceHeatBalance - spsz output is now fully populated
5ZoneAirCooledWithSpacesHVAC - spsz output fully populated before and after fix

@rraustad
Copy link
Contributor

rraustad commented Feb 21, 2025

This probably doesn't need any attention but I noticed that, for 5ZoneAirCooledWithSpaceHeatBalance which has auto generated spaces, the header for epluszsz and eplusspsz look the same. Reported as Zone/Space Name ":" Design Day Name ":" Variable Name, so am just noting it here.

image

Then I checked eplusout.csv and the spaces have their own report names which allows proper reporting (i.e., if they had the same report name they would be duplicates and excel would likely not show the duplicate, they would be in the csv but excel might? get confused). Again, probably nothing to do here.

image

UPDATE: I have noticed this "duplicate" issue before but it happened when the cooling design day and heating design day date were the same (e.g., 1/21 for cooling and heating), so the header should report accurately). It was the rows that did not show up correctly, not the columns.

@rraustad
Copy link
Contributor

Tested 5ZoneAirCooledWithSpaceHeatBalance before and after this fix. This is eplusspsz.csv:

image

@rraustad
Copy link
Contributor

Also tested 5ZoneAirCooledWithSpaceHeatBalance with Do Space Heat Balance for Sizing set as Yes and No. eplusspsz.csv is blank when set to No so this works as expected.

ZoneAirHeatBalanceAlgorithm,
  ThirdOrderBackwardDifference,  !- Algorithm
  No,                      !- Do Space Heat Balance for Sizing
  Yes;                      !- Do Space Heat Balance for Simulation

image

@rraustad
Copy link
Contributor

rraustad commented Feb 21, 2025

Just out of curiosity of why the unit test didn't work, at the end of writeZszSpsz the spsz file is closed. Just before it's closed os = unique_ptr, after it's closed os = empty. It's the os pointer that is getting tested in state->files.spsz.get_output().empty()

image

image

if (forSpaces) {
zoneNum = state.dataHeatBal->space(i).zoneNum;
}
if (!state.dataHeatBal->Zone(zoneNum).IsControlled) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Only if you have any other changes.

int zoneNum = (forSpaces) ? state.dataHeatBal->space(i).zoneNum : i;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course. That construct still is not my first thought, or second, or ...

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.

These changes correct the issue.

Copy link
Contributor Author

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

Walkthru

@@ -219,25 +219,6 @@ void ManageSizing(EnergyPlusData &state)
state.dataGlobal->ZoneSizingCalc = true;
Available = true;

if (state.dataSize->SizingFileColSep == CharComma) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Move to ZoneEquipmentManager to avoid creating an empty spsz output file. Can't make that decision here yet, because the project data (with Do Space Heat Balance flags) has not been read yet.

@@ -2346,14 +2346,15 @@ std::string sizingPeakTimeStamp(EnergyPlusData const &state, int timeStepIndex)
void writeZszSpsz(EnergyPlusData &state,
EnergyPlus::InputOutputFile &outputFile,
int const numSpacesOrZones,
Array1D<DataZoneEquipment::EquipConfiguration> const &zsEquipConfig,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is no longer needed.

Comment on lines -2356 to +2357
if (!zsEquipConfig(i).IsControlled) continue;
int zoneNum = (forSpaces) ? state.dataHeatBal->space(i).zoneNum : i;
if (!state.dataHeatBal->Zone(zoneNum).IsControlled) continue;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sizing output, check if the parent zone is controlled instead of checking zsEquipConfig(i).IsControlled (which is only true if there are SpaceHVAC:* objects).

} else {
state.files.spsz.filePath = state.files.outputSpszTxtFilePath;
}
state.files.spsz.ensure_open(state, "UpdateZoneSizing", state.files.outputControl.spsz);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait until here to open zsz and spsz files. At this point we know if doSpaceHeatBalanceSizing is active.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually glad you fixed this. I saw the 0 kB files but wasn't sure how to avoid that. This is a perfect fix.

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 IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
4 participants