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

Implement new coupled basement simulation model #4514

Merged
merged 77 commits into from Nov 24, 2014
Merged

Conversation

mitchute
Copy link
Collaborator

@mitchute mitchute commented Oct 6, 2014

Pull request extending zone-slab coupling work to zone-basement coupling.

sushobhit and others added 30 commits August 18, 2014 11:25
…nergyPlusTeam into 1286412-Basement

Conflicts:
	src/EnergyPlus/PlantPipingSystemsManager.cc
… 1286412-Basement

Conflicts:
	src/EnergyPlus/PlantPipingSystemsManager.cc
…Team into 1286412-Basement

Conflicts:
	src/EnergyPlus/DataPlantPipingSystems.cc
	src/EnergyPlus/DataPlantPipingSystems.hh
	src/EnergyPlus/PlantPipingSystemsManager.cc
…es' into 1286412-Basement

Conflicts:
	idd/Energy+.idd
@mitchute
Copy link
Collaborator Author

@mjwitte I can update that. I think @Myoldmopar fixed a few things in that file and renamed it ZoneCoupledGroundHTBasement.idf, so this file is going to go away. I just noticed that it is still there. Nevertheless, this change should apply to both.

@mjwitte
Copy link
Contributor

mjwitte commented Nov 10, 2014

@mmAtTs @Myoldmopar Are you planning a similar name change for the slab
examples? I noticed they still have version=8.1, the new
ZoneCoupledGroundHTBasement.idf is 8.2.

Can we come up with a better set of inputs here?

 5,                       !- Kusuda-Achenbach Average Surface 

Temperature {C}
0, !- Kusuda-Achenbach Average Amplitude of
Surface Temperature {C}
0, !- Kusuda-Achenbach Phase Shift of Minimum
Surface Temperature {days}

When I ran this and got near-zero cooling energy (probably because of
the above inputs) I was curious about the geometry of the building so I
opened it in SketchUp Legacy OS Plugin and saw that two of the walls
were showing white which usually means they are backwards. Table output
shows no north area, and double the south area. So, the north walls are
backwards.

Ran the example file as-is, without horiz insul, and without any
insulation. Heating energy use went up - that's good. That's all for now.

On 11/10/2014 12:45 PM, Matt Mitchell wrote:

@mjwitte https://github.com/mjwitte I can update that. I think
@Myoldmopar https://github.com/Myoldmopar fixed a few things in that
file and renamed it ZoneCoupledGroundHTBasement.idf, so this file is
going to go away. I just noticed that it is still there. Nevertheless,
this change should apply to both.


Reply to this email directly or view it on GitHub
#4514 (comment).

Matt Mitchell added 2 commits November 11, 2014 09:50
…ntation in the slab and basement files. Updated descriptions in all files.
@mitchute
Copy link
Collaborator Author

@mjwitte I wasn't planning on changing the names, but I'm open to it. These latest commits should address the issues you pointed out, along with a few others I discovered.

@mjwitte
Copy link
Contributor

mjwitte commented Nov 11, 2014

@mmAtTs @Myoldmopar Seems we should either name all three (new slab and basement) examples starting with ZoneCoupledGroundHT or revert this one back to BasementWithInsulation?

@mitchute
Copy link
Collaborator Author

@mjwitte
Copy link
Contributor

mjwitte commented Nov 11, 2014

Maybe, maybe not. It's part of the 5ZoneAirCooled series, so this one makes sense. Not sure we really need 3 example files for the slab feature, but I'm not sure we have a policy established regarding proliferation of example files.

@mitchute
Copy link
Collaborator Author

@mjwitte @Myoldmopar I went ahead and renamed the two slab files for consistency.

@Myoldmopar
Copy link
Member

@mmAtTs Looks like the files got renamed, but the cmake rules didn't get updated based on these failures. I'll get it updated in testfiles/CMakeLists.txt.

@Myoldmopar
Copy link
Member

@mmAtTs Does that commit (beef4b4) look like the correct filename changes?

@nrel-bot
Copy link

@mmAtTs @lgentile it has been 7 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@mmAtTs I merged develop into this branch, and there were a few funny conflicts. I think I addressed them all, and the new file runs fine. I'm going to let CI have a pass at this, hopefully cleaning up the issues that aren't related to your work.

@Myoldmopar
Copy link
Member

@mmAtTs The code looks good. Transition rules are in place and the affected idfs have been transitioned with the new slab object name. I ran a test file and found that the error file is still providing warnings about roof/ceiling upside-down. I'm going to remedy that and make one more check-in. I'm going to go back over the doc changes once more also. I think that once these things and the CI machines are done, if all is well, this should be ready to merge.

@mjwitte Did you have anything further you'd like to review before this gets merged (today/tomorrow)?

@mitchute
Copy link
Collaborator Author

@Myoldmopar I thought I had all of that fixed up, but thanks for catching it.

@Myoldmopar
Copy link
Member

Alright, that last commit (beefe29) cleaned up those warnings, and the other two slab files have clean error files.

@Myoldmopar
Copy link
Member

I forgot I had already reviewed the docs. I think this is merge-able as long as CI doesn't complain. And the Clang build just completed with just a slab file diff only. So far so good.

Myoldmopar added a commit that referenced this pull request Nov 24, 2014
@Myoldmopar Myoldmopar merged commit 1c3d55d into develop Nov 24, 2014
@Myoldmopar Myoldmopar deleted the 1286412-Basement branch November 24, 2014 21:07
@mjwitte
Copy link
Contributor

mjwitte commented Nov 24, 2014

@Myoldmopar Found some errors in the idd by opening in IDFEditor. Was just about to commit. So, now what?

@Myoldmopar
Copy link
Member

Commit and push it anyway, it will create a new branch up here by the same name, just as though you had started a new branch from scratch. I am curious if Github will connect this closed pull request to that branch or not. We'll see.

@mjwitte
Copy link
Contributor

mjwitte commented Nov 24, 2014

You sure? I could slightly rename the branch.

@Myoldmopar
Copy link
Member

Renaming is fine, too. If you have the changes locally but haven't committed them, stash would be a nice way to do this also:

  • "stash save" the changes you've made which will revert the branch back to the state it was in during the last pull
  • Checkout develop locally, and do a Git Pull. Now your develop branch will have the basement stuff since it got merged.
  • Create/check out a new branch called basement-idd-fix
  • Do a "stash pop" to re-apply those changes you had previously stashed away. The IDD should be modified, and you'd be ready to commit and push.

@Myoldmopar Myoldmopar changed the title 1286412 basement Implement new coupled basement simulation model Mar 7, 2015
@Myoldmopar Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.

None yet

6 participants