-
Notifications
You must be signed in to change notification settings - Fork 394
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
[Refactor] Enums - Psychrometrics.hh #8864
Conversation
I would like to throw a couple more things into this branch if I can:
|
src/EnergyPlus/PsychCacheData.hh
Outdated
#ifdef EP_cache_PsyTwbFnTdbWPb | ||
cached_Twb.clear(); | ||
cached_Twb.fill(cached_twb_t()); | ||
#endif | ||
#ifdef EP_cache_PsyPsatFnTemp | ||
cached_Psat.clear(); | ||
cached_Psat.fill(cached_psat_t()); | ||
#endif | ||
#ifdef EP_cache_PsyTsatFnPb | ||
cached_Tsat.clear(); | ||
cached_Tsat.fill(cached_tsat_h_pb()); | ||
#endif | ||
#ifdef EP_cache_PsyTsatFnHPb | ||
cached_Tsat_HPb.clear(); | ||
cached_Tsat_HPb.fill(cached_tsat_h_pb()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::array
does not have a .clear()
method. So, I .fill()
ed them with the default constructor call for those structs.
src/EnergyPlus/Psychrometrics.cc
Outdated
#ifdef EP_cache_PsyTwbFnTdbWPb | ||
state.dataPsychCache->cached_Twb.allocate({0, twbcache_size}); | ||
state.dataPsychCache->cached_Twb.fill(cached_twb_t()); | ||
#endif | ||
#ifdef EP_cache_PsyPsatFnTemp | ||
state.dataPsychCache->cached_Psat.allocate({0, psatcache_size}); | ||
state.dataPsychCache->cached_Psat.fill(cached_psat_t()); | ||
#endif | ||
#ifdef EP_cache_PsyTsatFnPb | ||
state.dataPsychCache->cached_Tsat.allocate({0, tsatcache_size}); | ||
state.dataPsychCache->cached_Tsat.fill(cached_tsat_h_pb()); | ||
#endif | ||
#ifdef EP_cache_PsyTsatFnHPb | ||
state.dataPsychCache->cached_Tsat_HPb.allocate({0, tsat_hbp_cache_size}); | ||
state.dataPsychCache->cached_Tsat_HPb.fill(cached_tsat_h_pb()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.fill()
ed with the default constructor call for these structs.
src/EnergyPlus/Psychrometrics.hh
Outdated
@@ -845,10 +844,10 @@ namespace Psychrometrics { | |||
// values from PsyRhFnTdbWPb | |||
|
|||
// FUNCTION PARAMETER DEFINITIONS: | |||
static std::string const RoutineName("PsyRhFnTdbRhov"); | |||
static constexpr std::string_view RoutineName(PsyRoutineNames[static_cast<int>(PsychrometricFunction::RhFnTdbRhov)]); // PsyRhFnTdbRhov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep the static
keyword? I left RoutineName
s as separate variables for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me. There are a couple tiny comment-only changes that don't necessarily need to hold up the PR. If anyone else has any comments to add, and any commits are needed, then go ahead and clean those comments at the same time. Otherwise I'll do final testing and see if we can get this merged directly.
#ifdef EP_cache_PsyTsatFnHPb | ||
constexpr int tsat_hbp_cache_size = 1024 * 1024; | ||
constexpr int tsat_hbp_precision_bits = 28; | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
constexpr
FTW!
@@ -121,40 +163,28 @@ struct PsychrometricCacheData : BaseGlobalStruct | |||
{ | |||
|
|||
#ifdef EP_cache_PsyTwbFnTdbWPb | |||
Array1D<cached_twb_t> cached_Twb; // DIMENSION(0:twbcache_size) | |||
std::array<cached_twb_t, twbcache_size> cached_Twb; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, very very much better!
"PsyWFnTdbTwbPb", | ||
"PsyTsatFnPb", | ||
"PsyTwbFnTdbWPb_cache", | ||
"PsyPsatFnTemp_cache"}; // 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this initialization, I wonder if the comment block at the end is really necessary. I suggest either moving the comments up to each individual element, or delete the comment block if not necessary.
false, | ||
false, | ||
true, | ||
true}; // PsyTdpFnTdbTwbPb 1 | PsyRhFnTdbWPb 2 | PsyTwbFnTdbWPb 3 | PsyVFnTdbWPb 4 | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with this comment block. On this one it is probably worth pulling each one up to its associated element for clarity.
@@ -513,7 +512,7 @@ namespace Psychrometrics { | |||
// ASHRAE handbook 1993 Fundamentals, | |||
|
|||
#ifdef EP_psych_stats | |||
++state.dataPsychCache->NumTimesCalled(iPsyRhFnTdbRhovLBnd0C); | |||
++state.dataPsychCache->NumTimesCalled[static_cast<int>(PsychrometricFunction::RhFnTdbRhovLBnd0C)]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is the pattern we are going for, so this is good.
I pulled develop in, and I want to wait until the GHA alternate build configurations run on this branch because some of the conflicts were instead of #ifdef scopes that I'm not building. If it is happy, I believe everything is good to go here. |
So far so good. I'm going to go ahead and let Decent catch up, but I suspect I'll merge this either tonight or tomorrow morning. Thanks @jmythms ! |
Well, unfortunately develop moved again. Those diffs are unrelated. I can run regressions locally on this branch to get a good set of results. |
I just built and ran the full test suite on my machine and got zero diffs. This is good to go. Thanks @jmythms |
Thank you! |
Pull request overview
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.