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

Steam features part 1 (#9260) follow-up #10222

Merged
merged 22 commits into from
Sep 20, 2023
Merged

Steam features part 1 (#9260) follow-up #10222

merged 22 commits into from
Sep 20, 2023

Conversation

dareumnam
Copy link
Collaborator

@dareumnam dareumnam commented Sep 15, 2023

Pull request overview

  • Follow-up of Implement steam features Part 1 #9260
  • Addressed remaining comments from Implement steam features Part 1 #9260
  • Remove ExteriorEnergyUse::ExteriorFuelUsage and replace it with Constant::eFuel (the combined fuel type enum)
  • Fixed DistrictHeating:Steam calculate() (Added sensible heat part)
  • Added OutsideEnergySources.unit.cc and PlantLoadProfile.unit.cc and unit tests
  • Updated document

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

Pull Request Author

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)
  • 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

This will not be exhaustively relevant to every PR.

  • 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

@dareumnam dareumnam added Defect Includes code to repair a defect in EnergyPlus Documentation Related primarily on the LaTeX-based EnergyPlus documentation Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Sep 15, 2023
@dareumnam dareumnam added this to the EnergyPlus 23.2 milestone Sep 15, 2023
@dareumnam dareumnam self-assigned this Sep 15, 2023
Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

OK, this is fine, lots of good documentation. I don't have time to scrutinize every line of it, but it's certainly good to have it in place for this release. There are a couple TeX warnings that need to be cleaned up, luckily it's a 1 minute fix. I'm marking it request changes, but if you need me to fix those labels, just let me know and I can do it.

@@ -568,83 +564,6 @@ namespace ExteriorEnergyUse {
}
}

void ValidateFuelType(EnergyPlusData &state,
Copy link
Member

Choose a reason for hiding this comment

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

A function removed. Nice.

@@ -325,21 +325,19 @@ void OutsideEnergySourceSpecs::initialize(EnergyPlusData &state, Real64 MyLoad)
// The mass flow rate could be an inter-connected-loop side trigger. This is not really the type of
// interconnect that that routine was written for, but it is the clearest example of using it.

auto &loop = state.dataPlnt->PlantLoop(this->plantLoc.loopNum);
Copy link
Member

Choose a reason for hiding this comment

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

Good reference here, helps readability for sure.

@@ -560,7 +553,7 @@ void OutsideEnergySourceSpecs::oneTimeInit_new(EnergyPlusData &state)
meterTypeKey = "DistrictHeatingSteam";
}
SetupOutputVariable(state,
reportVarPrefix + "Energy",
format("{}Energy", reportVarPrefix),
Copy link
Member

Choose a reason for hiding this comment

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

Although format might not be saving a lot here since it was just one concatenation, it's still a good move.

} else if (resourceType == "ELECTRICITYPRODUCEDONSITE") {
sResourceType = "ElectricityProduced";
} else if (resourceType == "SOLARWATERHEATING") {
sResourceType = "SolarWater";
} else if (resourceType == "SOLARAIRHEATING") {
sResourceType = "SolarAir";
} else {
ShowSevereError(state, format("Invalid input for PythonPlugin:OutputVariable, unexpected Resource Type = {}", resourceType));
ShowFatalError(state, "Python plugin output variable input problem causes program termination");
if (static_cast<Constant::eResource>(getEnumValue(Constant::eResourceNamesUC, resourceType)) != Constant::eResource::Invalid) {
Copy link
Member

Choose a reason for hiding this comment

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

Nice.

\subsubsection{Inputs}\label{inputs-17-006}

\paragraph{Field: Name}\label{field-name-16-006}

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple TeX labels which are repeated, throwing warnings. These need to be tidied up before merge. It looks like if you just modify these two labels it will be fixed. You can also just remove the label command entirely if these aren't being referenced anywhere.

Copy link
Member

Choose a reason for hiding this comment

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

👍

\subsubsection{Inputs}\label{inputs-17-006}

\paragraph{Field: Name}\label{field-name-16-006}

Copy link
Member

Choose a reason for hiding this comment

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

👍

@Myoldmopar
Copy link
Member

Thanks for the doc fix @dareumnam, I'll do a quick build with develop since I just merged a different PR.

@Myoldmopar
Copy link
Member

I noticed a failing unit test locally and confirmed Decent is also showing it, so I'll hold here. I think it's a unit test that @jmarrec just added, so let me know if we need to coordinate to discuss the needed change to the code or the test. Thanks!

@Myoldmopar
Copy link
Member

Thanks for fixing that @dareumnam, Windows is now all happy, and I'm going to merge this in. The Linux debug builds will "fail" but I don't want to waste their time repeating tests. Thanks!

@Myoldmopar Myoldmopar merged commit caca0d2 into develop Sep 20, 2023
@Myoldmopar Myoldmopar deleted the SteamPart1Followup branch September 20, 2023 16:03
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 Documentation Related primarily on the LaTeX-based EnergyPlus documentation Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants