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

Deglobalizing EnergyPlus #8642

Merged
merged 12 commits into from
Mar 23, 2021
Merged

Deglobalizing EnergyPlus #8642

merged 12 commits into from
Mar 23, 2021

Conversation

Myoldmopar
Copy link
Member

Pull request overview

This was one of the big remaining ones. Just because it's in nearly every file. The DataIPShortCuts. I had hoped to refactor it at the same time to eliminate the use of the global variables, but after a couple hours I realized this was a terrible idea right now. The DataIPShortcuts are used in very fun ways throughout the code. This way it does the same old deglobalization, but should show no diffs. It is clean locally on ~100 regression tests.

@Myoldmopar Myoldmopar added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Mar 22, 2021
@Myoldmopar
Copy link
Member Author

It looks like there could be a very tiny VRF err output diff, which I have now fixed in my next commit that I'll push soon.

@Myoldmopar
Copy link
Member Author

Ended up deglobalizing a lot more modules here:

  • DataIPShortcuts
  • heat bal finite diff
  • data hvac controllers
  • ground temp model manager
  • data global constants
  • hvac cooled beam
  • hvac dx assisted coil
  • HWBaseboardRadiator
  • electric baseboard radiator
  • humidifiers
  • dataoutputs
  • hvac dx system

I'll fix up the test issues and get this cleaned up shortly.

@Myoldmopar
Copy link
Member Author

The debug failures are all fixed up. Windows builds are interesting, I'm getting a memory access violation in the RE2 destructor as it tries to clean up. EnergyPlus has already completed successfully, but when the destructor executes in RE2 it hits a bad memory spot. I'm bisecting on Windows now. Hopefully clear it up shortly.

@Myoldmopar Myoldmopar changed the title Deglobalizing DataIPShortcuts Deglobalizing EnergyPlus Mar 23, 2021
@Myoldmopar
Copy link
Member Author

OK, I verified the Windows issue locally and was able to isolate which commit caused the problem and I reverted it. I'll push it and see if Windows agrees.

@Myoldmopar
Copy link
Member Author

OK, not sure when CI is going to get around to my commit. I feel pretty confident after being able to reproduce the issues locally and getting them clean. Given the timing, I'm going to merge this in so we can move on.

@Myoldmopar Myoldmopar merged commit 9b8155b into develop Mar 23, 2021
@Myoldmopar
Copy link
Member Author

Yes, Windows was happy, now as long as integration is happy too then I made the right decision to merge this!

@Myoldmopar Myoldmopar deleted the ContinuedDeglobalizing branch March 23, 2021 23:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants