Report some electricity usage in kW W or kWh in IP units options

[jcyuan2020]

Pull request overview

• Tries to fix Proper IP units for reporting lighting electricity rate and electricity usage (energy) #9988
• Keeps some electricity usage in reporting with IP-Unit system options to still being report in kW or KWh
• A new unit convertion choice named InchPoudExceptElectricity are added for tabular and SQL outputs. With the option, the electricity usage are presented in kWh in an inch-pound environment (and some electricity power rates W)---this is more closely aligned with the usual conventions found in utility bills.
• Separating the predefined tables are generally a challenged to do at this stage; therefore they are not specifically adjusted this change since the information is too granule to capture if it is electric usage---and some of them are electric type but in some occurrences—e.g. sizing, it is arguable to say if this should be kept using Watt or should it be converted to the IP equivalent. However, they are already some built-in unit conversion mechanism capability with regular InchPound unit conversion choice (e.g. based on heading texts) ---the new choice of InchPoundExceptElectricity also inherited the same treatment method.

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.

• Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
• Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
• Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
• Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
• If any diffs are expected, author must demonstrate they are justified using plots and descriptions
• If changes fix a defect, the fix should be demonstrated in plots and descriptions
• If any defect files are updated to a more recent version, upload new versions here or on DevSupport
• If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
• If structural output changes, add to output rules file and add OutputChange label
• If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

• Perform a Code Review on GitHub
• If branch is behind develop, merge develop and build locally to check for side effects of the merge
• If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If feature, test running new feature, try creative ways to break it
• CI status: all green or justified
• Check that performance is not impacted (CI Linux results include performance check)
• Run Unit Test(s) locally
• Check any new function arguments for performance impacts
• Verify IDF naming conventions and styles, memos and notes and defaults
• If new idf included, locally check the err file and other outputs

[nrel-bot-3]

@Myoldmopar @jcyuan2020 @Myoldmopar it has been 28 days since this pull request was last updated.

[Myoldmopar]

@jcyuan2020 I pulled this branch and started resolving conflicts, but they became a little more than I was expecting. Could you take a pass at pulling develop in and resolving them? They will likely be super easy since you were the one making these changes. If you aren't able to, I'll give it another dedicated pass. Thanks!

[jcyuan2020]

@Myoldmopar Sure. I am starting to get these resolved.

[Myoldmopar]

OK, so this is almost all good. I had two comments that you can respond to, either in reply, or code changes:

• I dislike the magic number 1 being used in the electricity detection. Do we not have a named variable that already points to the electricity resource type? I'd love to have that dynamically defined so that if we ever change the resource type numbers, it won't break that calculation.
• With the many repeated checks on if (ip or ip_except_elec) being added in ORT, I feel like it's worth doing it as nicely as possible. In this case, I do think a worker method would be a very nice touch. Let me know what you think.

[Myoldmopar]

OK, so here is where we are adjusting it for electricity or not. The iResource as 1 and 13 look.....funny. Is there really not a named enum we can use for (iResource == 1) ?

[jcyuan2020]

I do feel that using enums (or values converted from enum to int) should be the way to go in general. However I would also tread with caution for now, for two reasons. 1. from the original definition, it has a mix of number 14 and EndUse:Num (which is also 14). This means that there are some initial activities started on changing this to use enum already---but probably not complete for some reasons (probably due to the complexity in this piece of code?):

2. There are some mapping which is not perfectly aligned for the index e.g. from 1 (electricity) ->1; 2->2, demonstrated in the following lines:
   

There you can observe that the index mapping from one vector (left) to that for the other vector (or multi-dim array/vector) is not completely aligned. There are 3->6; 4-8, and 5->9; and more. There are at least a few more blocks that look like this.

This in one hand probably does mean that using enum could be a better solution for some scenarios in this big file. But on the other hand, it also added complexity to go over the code to make sure at each these are done correctly for all of them.

Probably it would be an overall effort of refactoring the numbers like this in this file to enum. (I think @amir at some point also mentioned about this last year). I generally feel this undertake might be a separate refactoring pr or something like that.

[amirroth]

The EndUse::Num stuff is from me, from the EndUse refactor PR. I didn't do the entire file (which is enormous) only the loops that I actually changed.

Similarly, I am not suggesting you do this as part of this PR. This should probably be a separate effort.

But it should be an effort because new people are going to need to add to and modify this module and they will also need to learn what all of these numbers mean. It's bad coding practice to use numbers instead of enum values, which have meaning that you don't have to look up. It's a form of self-documentation. Same reason for using meaningful names for variables and functions.

[Myoldmopar]

This repeated pattern is probably fine. I'm inclined to say you could add a little worker method to the ort class to clean this code up. Consider this example:

#include <iostream>

enum class UnitsStyle {InchPound, InchPoundExceptElectricity, SI};

struct ORT {
    UnitsStyle unitsStyle;
    ORT(UnitsStyle us) : unitsStyle(us) {}
    bool ip() const {
        return this->unitsStyle == UnitsStyle::InchPound || this->unitsStyle == UnitsStyle::InchPoundExceptElectricity;
    }
};

int main() {
    ORT ort1(UnitsStyle::InchPound);
    std::cout << ort1.ip() << std::endl;
    ORT ort2(UnitsStyle::InchPoundExceptElectricity);
    std::cout << ort2.ip() << std::endl;
    ORT ort3(UnitsStyle::SI);
    std::cout << ort3.ip() << std::endl;
}

That way all of these if checks would just be replaced with:

if (ort->ip()) {

Which is a lot less to digest. Notes:

• I didn't look up the actual ort struct name, that's just a quick example
• I'm not saying ip is really the right function name, just a quick guess

Do you think this change is worth it here?

[jcyuan2020]

I think I got the idea here. Probably worth a try---except the "ORT" struct name could be a little bit confusing here as there are many similar names of "ort" or something alike that are already used in this(these) file(s).

[Myoldmopar]

The ORT is just a dummy name I put in that sample code. Take the ort variable...it is some type. Is it an instance of a struct OutputReportTabular? If so, then my example is meant to add a method as such:

struct OutputReportTabular {
  bool ip() const {
    /// etc.
  }
}

Basically - add the new method to whatever struct the ort variable is.

[Myoldmopar]

Some of this testing feels unrelated, but maybe I'm missing something. Anyway, it looks like at least some of the rates are being compared properly, so I'll take it.

[amirroth]

Why not just replace uses of NotFound with Invalid?

[jcyuan2020]

That could be from the legacy---NotFound was the original way of dealing with blacks or something invalid before the enum ear and here it was assigned to a higher value (than all valid entries). Then Invalid becomes the standard values to be included in an enum as -1. Probably it is time to revisit again to see--actually I do find a note three years ago (likely from me myself) about the use of NotFound and seemed to have some reason to keep it at that time:

[amirroth]

The compiler is going to do this for you anyway, but for readability you should probably define Real64 convFactor = ipElectricityConversionFactor / largeConversionFactor;. At that point you could also multiply rather than divide which is both faster and higher bandwidth, i.e., the processor has more multipliers than dividers because multiplication is common and also each multiplier is faster than each divider.

[jcyuan2020]

Thanks! A good point to use multiplication whenever possible.

[amirroth]

I'm going to have to do something about this. Not today though.

[amirroth]

You don't need to fix this now but this 1, 2, 6, 8 business is not good. At all.

[Myoldmopar]

This isn't quite right. I'll fix it up in the morning.

[Myoldmopar]

Everything works locally, and the ip() function is fixed up. It could be enhanced further for the remaining locations, but that can certainly come later. If CI is happy with my last commit I think this can go right in.

[Myoldmopar]

@jcyuan2020 this is what I meant. Add a member method to the existing ort-based structure, not adding a whole new struct.

[Myoldmopar]

This is what I wanted it to look like.

[Myoldmopar]

Keep in mind though, that the ip() function only checks the value of unitsStyle defined currently on the ORT state instance. So if you are comparing some other style, that function isn't the right one to use, so I just left these remaining ones as if(thisUnitsStyle == A || thisUnitsStlye == B)

[Myoldmopar]

Alright, merging this right in. Thanks @jcyuan2020 and all reviewers.