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

Adding stubs for init_state #10560

Merged
merged 9 commits into from
Jun 24, 2024
Merged

Adding stubs for init_state #10560

merged 9 commits into from
Jun 24, 2024

Conversation

amirroth
Copy link
Collaborator

I created stubs for an init_state(EnergyPlusData &state) function that is the mirror of the clear_state() function and that is intended to allow us to incrementally serialize and simplify our input and data-mining architecture. I have not actually implemented any of that yet, so this is really just empty functions with one exception. I moved a call to GetFluidPropertiesData() into FluidPropertiesData::init_state(). There is nothing to see here. These are not the droids you're looking for. Move along.

Mac and Windows are green, Ubuntu is not building due to the Python version update.

@amirroth amirroth added DoNotPublish Includes changes that shouldn't be reported in the changelog Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring labels Jun 12, 2024
@amirroth amirroth added this to the EnergyPlus 24.2 milestone Jun 12, 2024
@amirroth amirroth requested a review from Myoldmopar June 12, 2024 14:43
@amirroth amirroth self-assigned this Jun 12, 2024
Copy link
Collaborator Author

@amirroth amirroth left a comment

Choose a reason for hiding this comment

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

Bada bing, bada boom.

@@ -183,9 +183,14 @@ struct AirLoopHVACDOASData : BaseGlobalStruct
std::vector<AirLoopHVACDOAS::AirLoopDOAS> airloopDOAS;
std::vector<AirLoopHVACDOAS::AirLoopMixer> airloopMixer;
std::vector<AirLoopHVACDOAS::AirLoopSplitter> airloopSplitter;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're going to see a lot of this.

void clear_state() override
{
*this = AirLoopHVACDOASData();
new (this) AirLoopHVACDOASData();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And a little bit of this.

@@ -567,4 +567,262 @@ void EnergyPlusData::clear_state()
this->files.ssz.close();
this->files.zsz.close();
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the uber-function.

@@ -662,6 +662,12 @@ struct FluidPropertiesData : BaseGlobalStruct
std::array<FluidProperties::cached_tsh, FluidProperties::t_sh_cache_size> cached_t_sh;
#endif

void init_state(EnergyPlusData &state) override
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only non-empty init_state function thus far.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that FluidProperties.cc did not change which means these can also be deleted everywhere, as well as the variable state.dataFluidProps->GetInput.

    if (state.dataFluidProps->GetInput) {
        GetFluidPropertiesData(state);
        state.dataFluidProps->GetInput = false;
    }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm keeping this pretty minimal for now (strange, I know). I'm going to rework the FluidProperties API and see the effects downstream before propagating this, but I will do that in the following PR.

@@ -209,6 +209,8 @@ namespace SimulationManager {
state.dataInputProcessing->inputProcessor->getNumObjectsFound(state, "RunPeriod:CustomRange") > 0 || state.dataSysVars->FullAnnualRun);
state.dataErrTracking->AskForConnectionsReport = false; // set to false until sizing is finished

state.init_state(state);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I decided to plant the call to the uber-function here. Is there a better place for it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears this allows calling all getInput functions before the simulation gets started. And yes, order of calls in EnergyPlusData::init_state is critically important. This will replace the isInputObjectUsed function down at line 229 which is also trying to get information prior to the simulation starting. So somewhere between line 212 and 229 seems appropriate to me. I think at the very least that this call should be moved down below OpenOutputFiles at line 214. Or there may be a need to reorganize calls between lines 214 and 229 where some of those lines could be moved to init_state or reorganized locally here depending on what each function does.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm following correctly, ideally, we want to split up getInput into two parts: read and cross-check. e.g. Read all of the inputs but don't do any checking at first (e.g. checking for a zone name). Then the input order won't be (or could be less of) an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since getInputs write to output files (err for example) this call should be moved below OpenOutputFiles before merging. getFluidProperties does call Show*Error so the err file needs to be open first.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It appears this allows calling all getInput functions before the simulation gets started. And yes, order of calls in EnergyPlusData::init_state is critically important. This will replace the isInputObjectUsed function down at line 229 which is also trying to get information prior to the simulation starting. So somewhere between line 212 and 229 seems appropriate to me. I think at the very least that this call should be moved down below OpenOutputFiles at line 214. Or there may be a need to reorganize calls between lines 214 and 229 where some of those lines could be moved to init_state or reorganized locally here depending on what each function does.

Okay, I can move this wherever and we can keep moving it if needed. For now, it's essentially an empty function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since getInputs write to output files (err for example) this call should be moved below OpenOutputFiles before merging. getFluidProperties does call Show*Error so the err file needs to be open first.

Did you already do this?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but it does need to move. I'm not sure about the other calls below but that will show itself, as well as order dependence, once things start moving to the new init_state function.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I just checked. OpenOutputFiles opens eso, eio, mtr, and bnd. I think the bnd (node list) needs to be open before calling getInput. The err file gets opened in initializeEnergyPlus and initializeAsLibrary which seems it's already open by this point. So this line still needs to move after OpenOutputFiles.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it, although it looked like it was already moved for some reason (that's why I thought you did it). I'm going to make this transition over time, this just lets me get started.

Copy link
Contributor

@rraustad rraustad Jun 13, 2024

Choose a reason for hiding this comment

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

Regarding: "split up getInput into two parts: read and cross-check" I think if we want to read inputs early we would want to know that data is valid and error checked so that there is no need for other checks elsewhere (i.e., if (zoneNum > 0), etc.). Just suggesting this does not move too fast other than trying to get a good order for getInput calls. Once that effort is well under way we could make decisions like that.

@Myoldmopar
Copy link
Member

(Applied Clang Format and pulled develop so Ubuntu would be happy)

Copy link
Contributor

@rraustad rraustad left a comment

Choose a reason for hiding this comment

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

Once these unrelated diffs are corrected and this init_state call in SimulationManager is moved this can merge.

@rraustad
Copy link
Contributor

image

@Myoldmopar
Copy link
Member

Alright, resolved the conflicts and everything looks happy. I'll let CI run to confirm nothing got messed up (it didn't) and then merge.

@Myoldmopar
Copy link
Member

Thanks @amirroth !

@Myoldmopar Myoldmopar merged commit 4d45705 into develop Jun 24, 2024
15 checks passed
@Myoldmopar Myoldmopar deleted the InitStateStub branch June 24, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog 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.

None yet

9 participants