Convert `int constexpr` to `enum` : EconomicLifeCycleCost.* #9170

jmythms · 2021-11-05T20:14:42Z

Pull request overview

Convert economic life cycle cost categories from int constexpr to enum.

I used plain enums in this PR so that I did not have to use static_cast.

Explaination for Table diffs:
The rows just got rearranged when I rearranged the enums. Rearranged the enums so I could use loops instead of unrolled code for generating the tables. Screenshot from the ABUPS (.HTML) file generated when using the 5ZoneEconomicsTariffAndLifeCycleCosts.idf file:

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.

jmythms · 2021-11-05T20:21:19Z

src/EnergyPlus/EconomicLifeCycleCost.cc

+ for (iInObj = 0; iInObj < elcc->numRecurringCosts; ++iInObj) {
 state.dataInputProcessing->inputProcessor->getObjectItem(state,
 CurrentModuleObject,
- iInObj,
+ iInObj+1,
 AlphaArray,
 NumAlphas,
 NumArray,


I had to use +1 for the third argument in the getObjectItem() function so the vector does not go out of bounds.

jmythms · 2021-11-05T20:27:30Z

src/EnergyPlus/EconomicLifeCycleCost.cc

+ elcc->CashFlow[CostCategory::TotEnergy].mnAmount(jMonth) = elcc->CashFlow[CostCategory::Energy].mnAmount(jMonth);
+ elcc->CashFlow[CostCategory::TotOper].mnAmount(jMonth) =
+ elcc->CashFlow[CostCategory::Maintenance].mnAmount(jMonth) + elcc->CashFlow[CostCategory::Repair].mnAmount(jMonth) +
+ elcc->CashFlow[CostCategory::Operation].mnAmount(jMonth) + elcc->CashFlow[CostCategory::Replacement].mnAmount(jMonth) +
+ elcc->CashFlow[CostCategory::MinorOverhaul].mnAmount(jMonth) + elcc->CashFlow[CostCategory::MajorOverhaul].mnAmount(jMonth) +
+ elcc->CashFlow[CostCategory::OtherOperational].mnAmount(jMonth) + elcc->CashFlow[CostCategory::Water].mnAmount(jMonth) +
+ elcc->CashFlow[CostCategory::Energy].mnAmount(jMonth);
+ elcc->CashFlow[CostCategory::TotCaptl].mnAmount(jMonth) = elcc->CashFlow[CostCategory::Construction].mnAmount(jMonth) +
+  elcc->CashFlow[CostCategory::Salvage].mnAmount(jMonth) +
+  elcc->CashFlow[CostCategory::OtherCapital].mnAmount(jMonth);
+ elcc->CashFlow[CostCategory::TotGrand].mnAmount(jMonth) =
+ elcc->CashFlow[CostCategory::TotOper].mnAmount(jMonth) + elcc->CashFlow[CostCategory::TotCaptl].mnAmount(jMonth);


Enums start at 0 so converted elcc->CashFlow from EPVector to std::vector.

If CashFlow is really indexed by an enum then you can convert it to std::array. This applies to all arrays indexed by enum.

Only part of CashFlow is indexed by an enum... It is more of a std::vector/Array1D like container that can change size based on the number of items in the IDF.

More details:
As shown in this line, numCashFlow is the size of CashFlow, :
elcc->numCashFlow = countOfCostCat + elcc->numRecurringCosts + elcc->numNonrecurringCost + elcc->numResourcesUsed;.
In that equation, countOfCostCat is always constant and equal to the number of enum values, and that part of CashFlow is indexed by the enum. The other terms (numRecurringCosts, numNonrecurringCost, numResourcesUsed) depend on the IDF.

Should I pull out the countOfCostCat part and convert that to std::array, and then continue down that path?

I see, thanks for that clarification. As you were.

amirroth · 2021-11-06T16:19:43Z

Is it worthwhile making comments on this module since it is tagged for deprecation?

mjwitte · 2021-11-06T16:29:58Z

I'm pretty sure we discussed deprecating ComponentCost:*, but not LifeCycleCost:*

amirroth · 2021-11-06T16:47:13Z

src/EnergyPlus/EconomicLifeCycleCost.hh

 Energy,
 NonEnergy,
 NotComputed,
+ Num
 };

 constexpr const char *MonthNames(int const &i)


This thing can be a constexpr std::array<std::string view, 12> lookup, it shouldn't be a function. It should also probably be in a central utility or calendar module.

amirroth · 2021-11-06T16:47:40Z

src/EnergyPlus/EconomicLifeCycleCost.cc

@@ -223,13 +223,13 @@ void GetInputLifeCycleCostParameters(EnergyPlusData &state)
 // \key BeginningOfYear
 // \default EndOfYear
 if (UtilityRoutines::SameString(AlphaArray(2), "EndOfYear")) {
- elcc->discountConvention = iDiscConv::EndOfYear;
+ elcc->discountConvention = DiscConv::EndOfYear;


getEnumerationValue

amirroth · 2021-11-06T16:47:57Z

src/EnergyPlus/EconomicLifeCycleCost.cc

@@ -390,33 +390,33 @@ void GetInputLifeCycleCostParameters(EnergyPlusData &state)
 // \key None
 // \default None
 if (UtilityRoutines::SameString(AlphaArray(6), "ModifiedAcceleratedCostRecoverySystem-3year")) {
- elcc->depreciationMethod = iDeprMethod::MACRS3;
+ elcc->depreciationMethod = DeprMethod::MACRS3;


getEnumerationValue

amirroth · 2021-11-06T16:48:21Z

src/EnergyPlus/EconomicLifeCycleCost.cc

@@ -498,39 +498,39 @@ void GetInputLifeCycleCostRecurringCosts(EnergyPlusData &state)
 // \key OtherOperational
 // \default Maintenance
 if (UtilityRoutines::SameString(AlphaArray(2), "Maintenance")) {
- elcc->RecurringCosts(iInObj).category = costCatMaintenance;
+ elcc->RecurringCosts[iInObj].category = CostCategory::Maintenance;


getEnumerationValue

amirroth · 2021-11-06T16:48:30Z

src/EnergyPlus/EconomicLifeCycleCost.cc

 // A3, \field Start of Costs
 // \type choice
 // \key ServicePeriod
 // \key BasePeriod
 // \default ServicePeriod
 if (UtilityRoutines::SameString(AlphaArray(3), "ServicePeriod")) {
- elcc->RecurringCosts(iInObj).startOfCosts = iStartCosts::ServicePeriod;
+ elcc->RecurringCosts[iInObj].startOfCosts = iStartCosts::ServicePeriod;


amirroth · 2021-11-06T16:49:00Z

src/EnergyPlus/EconomicLifeCycleCost.cc

@@ -690,39 +690,39 @@ void GetInputLifeCycleCostNonrecurringCost(EnergyPlusData &state)
 // A1, \field Name
 // \required-field
 // \type alpha
- elcc->NonrecurringCost(iInObj).name = AlphaArray(1);
+ elcc->NonrecurringCost[iInObj].name = AlphaArray(1);
 // A2, \field Category
 // \type choice
 // \key Construction
 // \key Salvage
 // \key OtherCapital
 // \default Construction
 if (UtilityRoutines::SameString(AlphaArray(2), "Construction")) {


amirroth · 2021-11-06T16:49:09Z

src/EnergyPlus/EconomicLifeCycleCost.cc

 ShowWarningError(state,
 CurrentModuleObject + ": Invalid " + state.dataIPShortCut->cAlphaFieldNames(2) + "=\"" + AlphaArray(2) +
 "\". The category of Construction will be used.");
 }
 // N1, \field Cost
 // \type real
- elcc->NonrecurringCost(iInObj).cost = NumArray(1);
+ elcc->NonrecurringCost[iInObj].cost = NumArray(1);
 // A3, \field Start of Costs
 // \type choice
 // \key ServicePeriod
 // \key BasePeriod
 // \default ServicePeriod
 if (UtilityRoutines::SameString(AlphaArray(3), "ServicePeriod")) {


…ng table diffs from formatting

…ables

…ithout Escalation)

…ith Escalation)

jmythms · 2021-11-15T20:19:56Z

src/EnergyPlus/EconomicLifeCycleCost.hh

+ constexpr std::string_view Total{"Total"};
+ constexpr std::string_view TotalUC{"TOTAL"};


Is there a better way to do this?

Maybe, but I'm not sure if it is worth it or not. C++ has a toupper function that operates on individual characters, but not on a string. So you'd have to iterate over the characters, and you'd need to construct a string while doing it. Maybe @lefticus has a fancy idea for how to do this most efficiently, but unless this is a widespread issue on this PR, I'd just put a pin in it. Getting it into constexpr string_view is already such an improvement over the past...

This is the fastest and cheapest way to do this unless the MakeUpper function can itself be made constexpr. I don't think it can, but it may be worth a quick test. Even then the implementation wouldn't be cheaper, it would have the same cost but maybe the code would be a bit clearer. Either way, it's not worth holding up this PR.

Thank you for the inputs and insights.
No, it's not a widespread issue, just felt that it felt a bit weird pulling these out as individual variables.

Will investigate changing MakeUpper into a constexpr function separately.

https://godbolt.org/z/nKcEf5eev I've given it 10 minutes and haven't figured out a path forward on it. I assume that means it is not possible. 😆 🤣 JK, but for real that's a starting point if anyone wants to try to make a clean solution.

Where did you learn how to program?

More seriously, makeUpperCase cannot be made constexpr because it has to allocate memory for the target string so this path is closed. Just leave as is for now.

Myoldmopar

This is really great. I don't see any red flags here, this is just tons of really great cleanup. I will do some final testing locally but as far as I can tell this is good to go in. If anyone else wants any specific tests/demos/changes, speak up soon.

Myoldmopar · 2021-11-18T16:02:59Z

src/EnergyPlus/EconomicLifeCycleCost.cc

- ShowWarningError(state,
- CurrentModuleObject + ": Invalid " + state.dataIPShortCut->cAlphaFieldNames(6) + "=\"" + AlphaArray(6) +
- R"(". "None" will be used.)");
+ elcc->depreciationMethod = static_cast<DeprMethod>(getEnumerationValue(DeprMethodNamesUC, UtilityRoutines::MakeUPPERCase(AlphaArray(6))));


@mjwitte maybe you can remind me here...can we not count on the input being all caps at this point? Assuming we stick with a case-insensitive input format, even for epJSON values, we should just make sure the input processor upper cases everything, even the epJSON blob. (Of course not the fields with \retaincase).

It would be really nice to just know for sure that the AlphaArray was already upper case then we could eliminate so many MakeUPPERCase function calls and clean up so many chunks of code.

At present, there's no global pass to push the input strings to UPPERcase. getObjectItem and getAlphaFieldValue return UPPERcase strings. Direct access of the JSON data does not.

I don't think that is strictly necessary. There is nothing wrong with makeUpperCase as long as you only do it once per string. The most important thing is to avoid case-insensitive comparisons and there are other ways to do that. See this issue.

That's what I was afraid of, but I also see a good possibility there. If the IP just makes one quick pass to upper case all the (non-retain-case) fields in the IDF or epJSON, then we would get rid of a huge amount of these MakeUPPERCase calls spread out all over the code. And also it would make it perfectly clear to the developers that the data will be upper case as soon as the input processor is done with it. No ambiguity that leads to extraneous string operations during input processing because the dev isn't sure if it will be upper case or not.

I think this would be a great discussion to have, not here, but soon. Thanks!

Thanks @amirroth, I think I am in full agreement that there is nothing wrong with MakeUPPERCase, as long as it is only done once, but the problem is that we aren't quite sure if it's been done or not, so it ends up being done extra times. If the input processor just made it upper case one time then we could be absolutely sure it's upper case and eliminate many MakeUPPERCase calls spread out of the code. Anyway, nothing for this PR. Thanks!

I agree. If MakeUpperCase is done in input processor by default that's even better, but what I was saying is that we can eliminate a lot of the expensive case-insensitive comparisons even with no changes to the input processor or JSON module, because the problem is not MakeUpperCase inherently, it's the fact that SameString essentially does a MakeUpperCase on both inputs and it is called by many search routines.

Myoldmopar · 2021-11-18T16:06:17Z

src/EnergyPlus/EconomicLifeCycleCost.cc

- elcc->NonrecurringCost(iInObj).category = costCatOtherCapital;
- } else {
- elcc->NonrecurringCost(iInObj).category = costCatConstruction;
+ elcc->NonrecurringCost[iInObj].category =


All these changes seem fine really. I definitely look forward to the BITF stuff becoming constexpr functions instead of macros.

Myoldmopar · 2021-11-18T16:07:01Z

src/EnergyPlus/EconomicLifeCycleCost.cc

@@ -303,7 +295,13 @@ void GetInputLifeCycleCostParameters(EnergyPlusData &state)
 // \key November
 // \key December
 // \default January
- elcc->baseDateMonth = MonthToMonthNumber(AlphaArray(4), 1);
+ elcc->baseDateMonth = getEnumerationValue(UtilityRoutines::MonthNamesUC, UtilityRoutines::MakeUPPERCase(AlphaArray(4)));


Nice! So much cleaner, and another function gone.

Myoldmopar · 2021-11-18T16:07:42Z

tst/EnergyPlus/unit/EconomicLifeCycleCost.unit.cc

+ EXPECT_EQ(9, getEnumerationValue(UtilityRoutines::MonthNamesUC, UtilityRoutines::MakeUPPERCase("October")));
+ EXPECT_EQ(10, getEnumerationValue(UtilityRoutines::MonthNamesUC, UtilityRoutines::MakeUPPERCase("November")));
+ EXPECT_EQ(11, getEnumerationValue(UtilityRoutines::MonthNamesUC, UtilityRoutines::MakeUPPERCase("December")));
+ EXPECT_EQ(-1, getEnumerationValue(UtilityRoutines::MonthNamesUC, UtilityRoutines::MakeUPPERCase("Hexember")));


I probably would have just removed this unit test entirely. But it's fine, it's extra checking of the getEnumerationValue function.

Got it, will remember next time!

Myoldmopar · 2021-11-18T16:08:43Z

src/EnergyPlus/EconomicLifeCycleCost.cc

+ offset = CostCategory::Num;
+ rowHead(jObj + 1) = elcc->CashFlow[offset + jObj].name;
+ switch (elcc->CashFlow[offset + jObj].Category) {
+ case (CostCategory::Maintenance):


No need to put parentheses around the case labels. No need to hold it up, but in the future don't add them.

Got it. Happened because I memorized this pattern. Will not add them in the future.

Myoldmopar · 2021-11-18T16:10:40Z

src/EnergyPlus/EconomicLifeCycleCost.hh

+
+ int constexpr SizeDepr(41);
+
+ constexpr std::array<std::array<Real64, SizeDepr>, static_cast<int>(DeprMethod::Num)> DepreciationPercentTable{


This isn't the easiest thing to read/maintain, but I think it's a net gain. The huge IF block, non-constexpr, assignments weren't great either. Maybe there is a better way to format the tabular data ... I dunno. No need to address it now.

Would it be better to split stuff up to improve readability? Something like,

constexpr std::array<string_view> smallTable1; constexpr std::array<string_view> smallTable2; constexpr std::array<std:array> bigTable {smallTable1, smallTable2};

Myoldmopar · 2021-11-18T16:35:08Z

src/EnergyPlus/EconomicLifeCycleCost.cc

- rowHead(14) = "Total Operation";
- rowHead(15) = "Total Capital";
- rowHead(16) = "Grand Total";
+ for (int CashFlowCostCategory = CostCategory::Maintenance; CashFlowCostCategory <= CostCategory::TotGrand; ++CashFlowCostCategory) {


I see, so this is why the table diffs appear. I think that is acceptable and these loops are much nicer than the manually listed out chunks of code.

Myoldmopar · 2021-11-18T17:00:24Z

Merging in 10 minutes unless someone speaks up otherwise.

jmythms · 2021-11-18T17:28:49Z

Thanks so much!

Convert `int constexpr` to `enum` : EconomicLifeCycleCost.*

jmythms added 16 commits November 2, 2021 13:41

[WontCompile]checkpoint1

a7c21b4

removed enum StartFromZero + cleanup whitespace

d8ca85d

removed countOfCostCat

2be8cdb

fix CashFlow EPVector to std::vector

1c3665f

convert RecurringCosts to std::vector

879a5f0

convert NonRecurringCosts to std::vector

4f734be

NonRecurringCosts: change deallocate() to clear()

ffe7bf8

cleanup enum class SourceKindType + fix unit tests

9910aba

switch (elcc->CashFlow[offset + jObj].SourceKind)

27908bc

switch case enum DiscConv

6a46fdd

switch case enum PrValKind

7c87ea0

constexpr std::array CostCategoryNames+DeprMethodNames

fb6d219

constexpr 2D std:array DepreciationPercentTable

1f352b4

clangFormat

90aa707

ToChangeLater - getObjectItem iInObj to iInObj+1

fbe9868

fix conflict + merge dev

6e45fbc

jmythms added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Nov 5, 2021

jmythms added this to the EnergyPlus 2022.1 milestone Nov 5, 2021

jmythms self-assigned this Nov 5, 2021

jmythms commented Nov 5, 2021

View reviewed changes

amirroth reviewed Nov 6, 2021

View reviewed changes

jmythms added 23 commits November 8, 2021 14:10

change escalationStartMonth to use getEnum, start at 0

bcf13f1

fix EconomicLifeCycleCost_MonthToMonthNumber

a264049

Removed MonthToMonthNumber function

dbda914

Removed MonthNames function

321fffd

ClangFormat

fa27614

fix indexing mistake

be3b0a8

fix indexing mistake in debugging code

bafdf94

fix NonrecurringCost category check

cbf49ea

fix RecurringCost category check

6e900dc

Add+Removed comments

91b9d02

Use std::array<string_view> instead of strings in tables

9b682be

ClangFormat

931254b

ResourceCostCategory::Num instead of 2, Clean commit before introduci…

830fbb2

…ng table diffs from formatting

Capital Cash Flow:Use std::array<string_view> instead of strings in t…

fc85faf

…ables

rearrange enums - checkpoint

c42b71a

rearrange enums - Fix column header space issue

3005455

rearrange enums - Rearrange strings, assign correct std::array

c1505fc

rearrange enums - Rearrange table: Operating Cash Flow by Category (W…

ed6536b

…ithout Escalation)

rearrange enums - Rearrange table: Operating Cash Flow by Category (W…

31a0d79

…ith Escalation)

rearrange enums - Rearrange table: Present Value by Category

18612b9

rearrange enums - Convert column header to for loops

f212036

Merge branch 'develop' into enumELCC

3db5acf

ClangFormat src

866d83b

jmythms commented Nov 15, 2021

View reviewed changes

Myoldmopar approved these changes Nov 18, 2021

View reviewed changes

Myoldmopar merged commit 8e834f8 into develop Nov 18, 2021

Myoldmopar deleted the enumELCC branch November 18, 2021 17:27

yujiex pushed a commit that referenced this pull request Jan 27, 2022

Merge pull request #9170 from NREL/enumELCC

d0207fc

Convert `int constexpr` to `enum` : EconomicLifeCycleCost.*

		constexpr std::string_view Total{"Total"};
		constexpr std::string_view TotalUC{"TOTAL"};


		int constexpr SizeDepr(41);

		constexpr std::array<std::array<Real64, SizeDepr>, static_cast<int>(DeprMethod::Num)> DepreciationPercentTable{

Convert int constexpr to enum : EconomicLifeCycleCost.* #9170

Convert int constexpr to enum : EconomicLifeCycleCost.* #9170

Conversation

jmythms commented Nov 5, 2021 • edited Loading

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmythms Nov 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirroth commented Nov 6, 2021

mjwitte commented Nov 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Nov 18, 2021

jmythms commented Nov 18, 2021

Convert `int constexpr` to `enum` : EconomicLifeCycleCost.* #9170

Convert `int constexpr` to `enum` : EconomicLifeCycleCost.* #9170

jmythms commented Nov 5, 2021 •

edited

Loading

jmythms Nov 9, 2021 •

edited

Loading

mjwitte commented Nov 6, 2021 •

edited

Loading