Address test failures on master

[lymereJ]

Pull request overview

Address test failures on master.

Review Checklist

This will not be exhaustively relevant to every PR.

• Perform a code review on GitHub
• All related changes have been implemented: method additions, changes, tests
• Check rubocop errors
• Check yard doc errors
• If fixing a defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
• If a new feature, test the new feature and try creative ways to break it
• CI status: all green or justified

[lymereJ]

Changing to straight EER to COP conversion, don't remove fan impact from metric since we don't to calculate a "cop no fan".

[lymereJ]

Removed the _with_fan suffix throughout to be consistent with other methods.

[lymereJ]

Incorrect reference.

[mdahlhausen]

@khaddad and @ckirney to review

[lymereJ]

@khaddad @ckirney @mdahlhausen - For some reason, the CI is not showing on GitHub that the build has finished but Jenkins shows that it has, with four failures:

All failures are the same:

Failure:
test_NECB2017_SecondarySchool_regression_Electricity(Minitest::Result) [/srv/data/jenkins/git/openstudio-standards/PR-1700/9/test/necb/building_regression_tests/tests/test_necb_bldg_SecondarySchool_NECB2017_Electricity.rb:21]:
{"diffs-errors"=>["For OS:Coil:Cooling:DX:SingleSpeed called 'CoilCoolingDXSingleSpeed_dx 13 36kBtu/hr 14.0SEER' field 'Rated COP': before model = 4.11713, after model = 3.8248"]}

Failure:
test_NECB2017_SecondarySchool_regression_NaturalGas(Minitest::Result) [/srv/data/jenkins/git/openstudio-standards/PR-1700/9/test/necb/building_regression_tests/tests/test_necb_bldg_SecondarySchool_NECB2017_NaturalGas.rb:21]:
{"diffs-errors"=>["For OS:Coil:Cooling:DX:SingleSpeed called 'CoilCoolingDXSingleSpeed_dx 13 36kBtu/hr 14.0SEER' field 'Rated COP': before model = 4.11713, after model = 3.8248"]}

I believe that these failures are expected:

• Before, the NECB code was using the SEER to COP "with fan" method which actually was a "no fan" conversion from Thornton et al. 2011.: eer = -0.0182 * seer * seer + 1.1088 * seer and cop = (eer / 3.413 + 0.12) / (1 - 0.12) which for this case give us -0.0182*14**2+1.1088*14=11.956 and (11.956/3.413+0.12)/(1-0.12)=4.11713
• This PR changes the SEER to COP input to be "no fan" using the 90.1-Appendix G/ECB method: -0.0076*SEER**2+0.3796*SEER which for this case give us -0.0076*14**2+0.3796*14 = 3.8248

Please, let me know if I missed anything, otherwise I think this is good to go. I'll updated this branch from master now.

[mdahlhausen]

Thanks @lymereJ. The CI machine was full. @wenyikuang cleared it up today. Should be working now.

Those changes are fine with me - I'll wait for @ckirney to approve and update the NECB regression models.

[khaddad]

@lymereJ The input NECB reference values are entered in a json file. In this case these values are in file './necb/NECB2011/data/unitary.json'. If possible we want to keep the same values here in the test expected results file as the ones in the json file on the input side. When we convert the input SEER values in standards we use the method that accounts for the effect of the fan ('seer_to_cop' now called). When we are checking an osm file for the correct values of COP, we need to convert this back to SEER. At this stage the COP does not include the effect of the fan anymore. In the test it seems we want to use the method 'cop_to_seer_no_fan' so we can keep the original values of SEER values in the expected results file?

[lymereJ]

The input NECB reference values are entered in a json file. In this case these values are in file './necb/NECB2011/data/unitary.json'. If possible we want to keep the same values here in the test expected results file as the ones in the json file on the input side.

As it stands, this PR doesn't touch the efficiency JSON files, or the expected results. The change you commented on is "outdated" (as per GitHub), I think it comes from a previous commit.

When we convert the input SEER values in standards we use the method that accounts for the effect of the fan ('seer_to_cop' now called). When we are checking an osm file for the correct values of COP, we need to convert this back to SEER. At this stage the COP does not include the effect of the fan anymore. In the test it seems we want to use the method 'cop_to_seer_no_fan' so we can keep the original values of SEER values in the expected results file?

I'm not sure I understand. As it stands, this PR takes the requirements and convert them for input to an OpenStudio model as gross COP values (impact of the fan on the efficiency rating has been removed) which I believe is what EnergyPlus expects. The only changes that were made to the NECB tests is to convert the gross COP values ("no fan") from the test models to either EER (cop_no_fan_to_eer) or SEER (cop_no_fan_to_seer) and compare them to the expected results which match what's in ./necb/NECB2011/data/unitary.json`. Does that match your expectations?

In the US, as per AHRI testing standards, rated SEER and EER are always "net" efficiencies, not "gross", meaning that they always include the impact of fan power.

[khaddad]

@lymereJ I must not have looked at the latest code. Yes it all is good then. It makes it easier when the inputs to the jsons match the test expected values. Thanks

[mdahlhausen]

@ckirney or @khaddad can you regenerate the NECB regression tests with the new COPs as noted in the conversation above?