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

Refactor Globals #7695

Merged
merged 104 commits into from
Apr 29, 2020
Merged

Refactor Globals #7695

merged 104 commits into from
Apr 29, 2020

Conversation

mitchute
Copy link
Collaborator

Initial workspace to test moving global vars to a single location to support the upcoming reset-state work.

@mitchute mitchute added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jan 17, 2020
@mitchute mitchute added this to the EnergyPlus Future milestone Jan 17, 2020
@mitchute
Copy link
Collaborator Author

@Myoldmopar @brianlball per discussions today, I made a design change and moved the clear_state to the header. We should think about whether this is the right path.

@nrel-bot-2b
Copy link

@mitchute @lgentile it has been 28 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@lefticus thank you very much for your support and your review. I'll try to address a few things:

Pass only the sub object necessary when you can.

Absolutely. We can include this in the current version of this branch as a prototype of limiting access to different parts of the state.

Pass by const & when possible

Yep, another one that will be easy and a precedent should be set. We should simply assume const until such point in the call stack that we need to modify it, and only be non-const from that point upward.

Merge OutputFiles and EnergyPlusData where possible.

I will very much enjoy seeing the point where OutputFiles just becomes another "part" of the this new "state" of EnergyPlus. I don't think we will address it in this current branch. For this branch, we aren't adding any more new stuff to this extracted state, just going to clean up what we've already done. As you are still making your output file changes, I'd like to just leave things as-is here except what we need to do, and then we can make a single effort to get OutputFiles onto the main state in one branch. After that you can proceed to do output files and we will be able to proceed on our stuff and it shouldn't conflict too much.

What is General.hh and why does it include EnergyPlusData.hh?

That file is an artifact of the Fortran conversion and the contents of that file could almost certainly be put into better locations. But I'm not sure it's a problem either.

TempSolveRoot::SolveRoot

The SolveRoot stuff is a bit hairy so I think anything in this area should just be considered a stop-gap solution that will be remedied once we have things fully fleshed out.

(Tabs and formatting)

Uh, good call. I wonder why the CI scripts are not finding the style problems.

If the code is no longer necessary, delete it.

Yup, agreed, we can go ahead and delete all the commented members now that we feel good about proceeding down this path.

@Myoldmopar
Copy link
Member

Uhh, CI looks exceedingly happy. 🍏 📗 🟢 💚

I think Windows took a nap, I'll try to wake him up soon, but anyway, this looks really really good. I like the latest changes where you took the advice of @lefticus and started passing in only portions of the state variable, not the whole thing. That's good stuff.

@kbenne
Copy link
Contributor

kbenne commented Apr 28, 2020

I am not an official reviewer, but I wanted to chime in and say that I strongly support this. This opens the door for new features that could be very exciting, including:

  • Save, reset, rewind state. Think about being able to stop, rewind, and replay parts of the simulation. Realtime optimization, MPC, Digital twin all come onto the table.
  • Multiple, parallel simulations.
  • Nicer code organization once you swallow the change of adding the argument to all of the functions. It is temporary pain for significant future reward.

@Myoldmopar
Copy link
Member

OK, this is very happy. Thanks for those who have reviewed this. Anyone have final thoughts? I'll likely merge this in the morning.

@mitchute
Copy link
Collaborator Author

I added a couple of review comments here and here. FYI @lefticus @Myoldmopar

Other than that, "I'm @mitchute and I approve these changes."

@Myoldmopar
Copy link
Member

Top notch everyone. I'm merging this.

@Myoldmopar Myoldmopar merged commit 9e88f63 into develop Apr 29, 2020
@Myoldmopar Myoldmopar deleted the global branch April 29, 2020 14:32
# for free to join this conversation on GitHub. Already have an account? # 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.

10 participants