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

Correct and update all emissions meter source names from 'Electric' to 'Electricity' for consistency. #9101

Merged
merged 6 commits into from
Dec 9, 2021

Conversation

matthew-larson
Copy link
Contributor

Pull request overview

  • Fixes ABUPS 'Total Source Energy' does not include Electricity when FuelFactors are used #9042
  • This pull request updates the reference meter names for emissions from electricity from using 'Electric' to now using 'Electricity'. The defined source type name is 'Electricity' so this keeps it consistent across the board.
  • The main cause of the issue was ort->sourceTypeNames(iResource) + "Emissions:Source" was being called in the GetInputOutputTableSummaryReports function, where sourceTypeNames(1) is "Electricity". So it was looking for meter "ElectricityEmissions:Source", but the meter being created was actually called "ElectricEmissions:Source", so it wasn't being found for the reports. Changing this meter and other related meters to use "Electricity" instead of "Electric" as it's prefix removes this discrepancy.
  • The unit test for meters was also updated to reflect this.

The below results are from the 'AirEconomizerFaults_RefBldgLargeOfficeNew2004_Chicago.idf' example file, which utilizes FuelFactors objects for the Source Energy calculations.

Develop
image

PR
image

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

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
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally

@matthew-larson matthew-larson added Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze) labels Sep 27, 2021
@matthew-larson matthew-larson added this to the EnergyPlus 2022.1 milestone Sep 27, 2021
@matthew-larson matthew-larson self-assigned this Sep 27, 2021
@matthew-larson matthew-larson marked this pull request as ready for review September 28, 2021 15:20
@nrel-bot-3
Copy link

@matthew-larson @lgentile it has been 28 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

@mjwitte, could you comment on this if you have a chance? I can do the full review, but I would love your input.

@mjwitte
Copy link
Contributor

mjwitte commented Nov 1, 2021

@Myoldmopar The changes look reasonable. Surprised (not!) that this slipped through during the great meter renaming of 1872 (seems that long ago). This will need a Report Variables 9-6-0 to ??-??-0.csv file listing each flavor of ElectricityEmissions; meter. (I'm not sure if wildcard * works for that list or not.)

@nrel-bot
Copy link

@matthew-larson @lgentile it has been 28 days since this pull request was last updated.

@Myoldmopar
Copy link
Member

So I pulled this branch, merged in latest develop, then went to the Report Variables file to add all the entries and get it transitioning properly. I tested out the transition utility on the _SmallOfficeDulles file, and it wasn't converting the output variable names. This seemed strange, so I debugged into the program, and it does find the Report Variables file, but it was showing zero report variable changes found in that file...which I had modified. I checked, and the Report Variables file inside the Products directory was blank...because CMake had not reconfigured. The CMake system appears to not respond to a change in that file and understand that it needs to re-copy it in. Rerunning CMake and executing the transition binary, and it started working....mostly. I am getting a bunch of pre-processor warnings coming out in the transitioned IDF. @mjwitte perhaps you could shed some light on this? If not I'll dig in a bit more:

Old:

Output:Meter:MeterFileOnly,ElectricEmissions:CO2,monthly;

Output:Meter:MeterFileOnly,ElectricEmissions:CO2,annual;

New:

Output:PreprocessorMessage,Conversion 9.6 => 22.1,Warning,
Output:Meter:MeterFileOnly (old)="ElectricEmissions:CO2",
 conversion to Output:Meter:MeterFileOnly (new)="ElectricityEmissions:CO2",
 has the following caution "ElectricityEmissions:CO2".;
 
Output:Meter:MeterFileOnly,ElectricityEmissions:CO2,monthly;

Output:Meter:MeterFileOnly,ElectricityEmissions:CO2,annual;

It is converting the output variable properly, I just don't know what it is complaining about. I'll push my branch here if I can, and if you happen to notice anything @mjwitte or anyone else, that would be great, but if not I'll figure it out.

@Myoldmopar
Copy link
Member

Nevermind, I don't seem to have the credentials to push to this branch. I guess I can send a patch via email like the good old days.

@mitchute
Copy link
Collaborator

mitchute commented Dec 8, 2021

I guess I can send a patch via email like the good old days.

I thought you still had an instance of StarTeam running in the background for times like this 😃

@Myoldmopar
Copy link
Member

@mjwitte and I secretly still do all our testing on a Fortran build stored in a StarTeam instance with ms word docs.

@mjwitte
Copy link
Contributor

mjwitte commented Dec 8, 2021

@mjwitte and I secretly still do all our testing on a Fortran build stored in a StarTeam instance with ms word docs.

Ahhhh, the good old days. Nostalgia is nice during the holiday season.

@mjwitte
Copy link
Contributor

mjwitte commented Dec 8, 2021

The cautions warnings are taken from the 3rd column of the csv (not that I remembered that).

old variable name,new variable name -- <DELETE> to delete,add variable names (before this line) and leave off units -- text in this column will add a warning,,,,,,

Examples from prior versions:
Report Variables 2-2-0-023 to 3-0-0.csv

PV Generator Inverter Power Delivered,Inverter AC Power Output,"<key mismatch if not wildcarded, so use *>"

Report Variables 7-0-0-036 to 7-1-0.csv

Surface Int Convection Heat Rate,Surface Inside Face Convection Heat Gain Rate,changed sign convention
Surface Int Convection Heat Rate per Area,Surface Inside Face Convection Heat Gain Rate per Area,changed sign convention

So, it sounds like column C is not empty?

@Myoldmopar
Copy link
Member

Thanks @mjwitte, that's super helpful.

@matthew-larson are you able to flip the switch so I can push my changes to this branch? Otherwise I'll have to push the branch to our own repo, and open a new PR, which is fine, but hopefully unnecessary.

@nealkruis
Copy link
Member

@Myoldmopar I can give you write permissions. Stand by...

@Myoldmopar
Copy link
Member

Alright, so the problem was that the third column "didn't exist." There was no trailing comma after the new report variable name, which is apparently problematic. So anyway, it's all set now and runs without adding preprocessor warnings. I would love someone to comment on the list of fields I added to make sure it is exhaustive, and otherwise if this is clean it can probably go right in. Thanks!

Comment on lines -1784 to +1786
Zone,Meter,ElectricEmissions:Nuclear High {[}kg{]}
Zone,Meter,ElectricityEmissions:Nuclear High {[}kg{]}
\item
Zone,Meter,ElectricEmissions:Nuclear Low {[}m3{]}
Zone,Meter,ElectricityEmissions:Nuclear Low {[}m3{]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do we really want a resource type with a space in it? "Nuclear High" and "Nuclear Low" have been there a long time. so I guess it works, but those may be the only meter name components with a space in them. I realize if we change that here, then it crosses into all of the end-use types for these two resources. There's also "Carbon Equivalent". If there's agreement to change these, it could be done as a separate issue.

Copy link
Member

Choose a reason for hiding this comment

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

I agree this looked weird, but I wasn't going to make that kind of change here. Feel free to open up discussion on a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

See #9207.

Comment on lines -1788 to +1790
Zone,Meter,PurchasedElectricEmissions:Source {[}J{]}
Zone,Meter,PurchasedElectricityEmissions:Source {[}J{]}
\item
Zone,Meter,SoldElectricEmissions:Source {[}J{]}
Zone,Meter,SoldElectricityEmissions:Source {[}J{]}
Copy link
Contributor

Choose a reason for hiding this comment

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

The report variables transition file needs these two meters as well.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, thanks for spotting these!

@Myoldmopar
Copy link
Member

Windows CI tumbled there, but it was unrelated. The rest of the CI results look good, just MDD and MTD style diffs. I'll add those meters that @mjwitte pointed out, and if there's nothing else then I think this can merge in.

@mjwitte
Copy link
Contributor

mjwitte commented Dec 9, 2021

@Myoldmopar This should probably get an entry in the output changes doc.

@Myoldmopar
Copy link
Member

Got the two meters added, and added an entry to the output changes doc. Should be good to go.

@mjwitte mjwitte mentioned this pull request Dec 9, 2021
3 tasks
@Myoldmopar
Copy link
Member

OK, merging this in. If there is anything else, it can be added to the new #9207 issue. Thanks all.

@Myoldmopar Myoldmopar merged commit 4b92261 into NREL:develop Dec 9, 2021
@Myoldmopar Myoldmopar deleted the fix-emissions-meter branch December 9, 2021 19:07
yujiex pushed a commit that referenced this pull request Jan 27, 2022
Correct and update all emissions meter source names from 'Electric' to 'Electricity' for consistency.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Defect Includes code to repair a defect in EnergyPlus NotIDDChange Code does not impact IDD (can be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ABUPS 'Total Source Energy' does not include Electricity when FuelFactors are used
10 participants