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

Simplify Fluid Caching #9946

Closed
wants to merge 5 commits into from
Closed

Simplify Fluid Caching #9946

wants to merge 5 commits into from

Conversation

amirroth
Copy link
Collaborator

@amirroth amirroth commented Apr 8, 2023

After the recent move of the Glycol specific heat cache to state and resulting slight slowdown, I tried a different caching approach in which each Glycol had its own small cache rather than all Glycol's sharing a single large one.

I didn't get the speedups I was looking for (in fact, there are some small "noise" slowdowns) but there are also some "big" diffs which seem unrelated.

This PR is not so much for merging at this point, it's for getting some help looking at those diffs.

@rraustad rraustad added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Apr 8, 2023
@rraustad
Copy link
Contributor

rraustad commented Apr 9, 2023

There is no good way to view diffs for this change. This is a cashing change, and if there were no cashing in E+ there woud be some base answer. And if caching was added to E+ that answer would not change significantly because that's the intent. The fact that zone temp changed by 0.2C in an example file is a clue to the fact this change has caused some diff. I don't understand why all example files did not have a change in zone temp but I expect it is a result of when cashing is used. I hope that these initial clues help get to the bottom of this. Results from CentralChillerHeaterSystem_Cooling_Heating, I don't think that these diffs are a good reason for thinking that there is something wrong with this change. Any change to the cashing method is bound to cause diffs of this magnitude.

image

The overall diffs for this file are pretty small:

Date/Time,Electricity:Facility [J](Monthly),Electricity:Plant [J](Monthly),Electricity:Facility [J](RunPeriod),Electricity:Plant [J](RunPeriod)
January,9.45660610828445e-06,9.80467198329661e-06,9.45660610828445e-06,9.80467198329661e-06
July,0.00039885698714821647,0.0008694885177699054,0.00039885698714821647,0.0008694885177699054

@rraustad
Copy link
Contributor

rraustad commented Apr 9, 2023

Diffs in plant outlet node:

image

And the plant outlet node set point temps are constant:

image

@amirroth
Copy link
Collaborator Author

amirroth commented Apr 9, 2023

@rraustad, thanks for looking into this. I am doing some simple local testing here and I am not convinced that the initial caching strategy we used was sound (specifically, it may have been giving bogus results for 0C temps). I am going to add a unit test to try to figure this out.

@rraustad
Copy link
Contributor

rraustad commented Apr 9, 2023

@amirroth if you do see something in the original implementation that isn't working correctly, you can fix that and then compare this branch to develop again.

@amirroth
Copy link
Collaborator Author

amirroth commented Apr 9, 2023

Yes. This implementation dates back to the Fortran days before we were doing proper unit tests. I don't think we really ever tested around the caching implementation to make sure it lined up well with uncached results in "all cases". Specifically, looking at what we did it appears that there was an implicit assumption that temperatures would be in Kelvin and so 0 would effectively be an invalid temperature and could be used to mean "this is an invalid cache entry". This is what I am trying to suss out now.

@rraustad
Copy link
Contributor

rraustad commented Apr 23, 2023

To get a sense of what these diffs are I compiled develop without caching, develop w caching, and this branch w caching and a single change of static int constexpr SpecificHeatCacheSize = 32 * 1024; per @amirroth.

image

@nrel-bot-2
Copy link

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

@rraustad
Copy link
Contributor

rraustad commented May 3, 2023

As a side note, I am seeing this warning from a Twb calculation from the new example file for #9979. I thought it was an issue with supervisory controls but it turns out the warning is incorrect.

** Warning ** Temperature out of range [-100. to 200.] (PsyPsatFnTemp)
** ~~~ ** Routine=NodeReportingCalc:VAV_3 SUPPLY EQUIPMENT OUTLET NODE,
** ~~~ ** Environment=ANNUAL, at Simulation time=02/06 18:15 - 18:30
** ~~~ ** Input Temperature=-149.27

The dry-bulb temperature is 7.8 C and humidity ratio is 0.00058 ? The actual WB temp is around -0.7 C, so during the iterations to find WB something is going very wrong.

From bnd file: Node,68,VAV_3 SUPPLY EQUIPMENT OUTLET NODE,Air,9

image

@Myoldmopar
Copy link
Member

@amirroth do you want to continue using this PR for investigating diffs and caching sensitivities? That's fine, of course, but if it was just a temporary test, I'll close it as I prune down the PR list.

@nrel-bot-2b
Copy link

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

@amirroth
Copy link
Collaborator Author

I don't have the ability to debug this kind of stuff on my own. My suspicion is that something is wrong with the current caching implementation, but I do not know how to suss that out.

@rraustad
Copy link
Contributor

I did look at a few variations of the caching model. For example these 2 model vars:

   static int constexpr SpecificHeatCacheSize = 1024 * 1024;
   double const t = Temperature + 1000 * GlycolIndex;

But I don't really know what I am doing here so not sure what these results tell me. I was just trying to see the difference between this model and develop with and without caching. The result changed each time. I wasn't expecting that.

Offestting t by kelvin:

image

Offsetting t by 1000 * GlycolIndex:

image

and changing cach size:

image

@amirroth
Copy link
Collaborator Author

SimplifyFluidCaching aside (which I think has a problem with 0C temperatures), the fact that develop shows different results with and without caching (assuming these are considered significant) is not good. Caching is not supposed to do that. Should probably get to the bottom of this first if possible.

@amirroth amirroth closed this May 18, 2023
@Myoldmopar Myoldmopar deleted the SimplifyFluidCaching branch July 22, 2024 14:16
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.

8 participants