Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Translate EMS Example Files to PythonPlugin #7915

Merged
merged 64 commits into from
May 7, 2020
Merged

Conversation

mitchute
Copy link
Collaborator

@mitchute mitchute commented Apr 8, 2020

Pull request overview

Translate the remaining EMS example files to PythonPlugin. Some current python example files don't mirror what the original EMS file does, so this should be checked and results between the two methods compared.

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

@mitchute mitchute added the NewFeature Includes code to add a new feature to EnergyPlus label Apr 8, 2020
@mitchute mitchute added this to the EnergyPlus 9.4.0 milestone Apr 8, 2020
@mitchute mitchute self-assigned this Apr 8, 2020
@mitchute mitchute marked this pull request as draft April 9, 2020 15:41
@Myoldmopar
Copy link
Member

And....assuming CI is OK with this last commit, we are done. This one was indeed a beast, but it seems to be working well. Spot checking a few zone temps reveals good agreement but then also things smoothing out like this:

image

There are small warmup convergence diffs, and some other minor stuff, but overall it seems to be in great shape.

@mitchute mitchute marked this pull request as ready for review May 7, 2020 12:02
@mitchute
Copy link
Collaborator Author

mitchute commented May 7, 2020

Agreed, @Myoldmopar. This is ready to go. I checked the diffs on this last file (DemandManager_LargeOffice) and it's the worst offender regarding math differences. Some smoothing in places but overall it looks good.

Var_0
Var_1
Var_2
Var_4
Var_5

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.

I'm going to make a commit with two things: some commented Python cleaned out, and the extra output variables added to the two EMS files cleaned out. It will not need to wait on CI for new results.

if (iCalledFrom == DataGlobals::emsCallFromBeginNewEvironment) {
BeginEnvrnInitializeRuntimeLanguage();
PluginManagement::onBeginEnvironment();
}
Copy link
Member

Choose a reason for hiding this comment

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

Added a new begin environment calling point in the plugins to clear trends and reset stuff. This is a big improvement.

plugin.run(iCalledFrom);
anyRan = true;
bool const didOneRun = plugin.run(iCalledFrom);
if (didOneRun) anyRan = true;
Copy link
Member

Choose a reason for hiding this comment

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

Properly return whether a plugin actually ran, not just if we tried to run plugins. This is another big improvement.

ADD_SIMULATION_TEST(IDF_FILE PythonPluginUserDefinedWindACAuto.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
ADD_SIMULATION_TEST(IDF_FILE PythonPluginWindowShadeControl.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
ADD_SIMULATION_TEST(IDF_FILE PythonPluginLrgOff_GridStorageSmoothing.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
ADD_SIMULATION_TEST(IDF_FILE PythonPluginCustomTrendVariable.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
Copy link
Member

Choose a reason for hiding this comment

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

Added gobs of new python plugin test cases that match the EMS cases. We are showing we can match the results of EMS which is a huge step.

@@ -2115,3 +2115,6 @@
HTML; !- Column Separator

!- =========== ALL OBJECTS IN CLASS: OUTPUT:VARIABLE ===========
Output:Variable,Zone1PTHPDXCoolCoil,Cooling Coil Total Cooling Rate,timestep;
Output:Variable,Zone2PTHPDXCoolCoil,Cooling Coil Total Cooling Rate,timestep;
Output:Variable,Zone3PTHPDXCoolCoil,Cooling Coil Total Cooling Rate,timestep;
Copy link
Member

Choose a reason for hiding this comment

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

Just added a few extra outputs to a couple EMS files for debugging, they aren't hurting anything but I may take them out if I need to do more commits.

self.Shade_Status_Off_handle = self.api.exchange.get_global_handle(
"Shade_Status_Off")

# self.Shade_Status_Interior_Shade_On_handle = self.api.exchange.get_global_handle(
Copy link
Member

Choose a reason for hiding this comment

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

All these commented pieces should be cleaned out.

@Myoldmopar Myoldmopar merged commit 0b207aa into develop May 7, 2020
@Myoldmopar Myoldmopar deleted the pyems_examples branch May 7, 2020 13:10
@Myoldmopar
Copy link
Member

Thanks @mitchute and @dareumnam, this is a major step for the python work.

@rraustad
Copy link
Contributor

rraustad commented May 7, 2020

I'm not sure but the plot of Core Top: Zone Mean Air Temperature makes me think of reduced system time steps and whether python is picking up information correctly during this time. A detailed simulation may show issues?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants