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

Autocalculate DOAS inputs in Sizing:Zone. Note I haven't figured out … #5557

Merged
merged 6 commits into from Mar 22, 2016

Conversation

Projects
None yet
6 participants
@wfbuhl
Copy link
Contributor

commented Mar 17, 2016

Pull request overview

Please change this line to a description of the pull request, with useful supporting information including whether it is a new feature, or fixes a defect, a cross reference to any defects addressed in this PR, the type of changes to be made, and whether diffs are expected/justified based on this change.

Work Checklist

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)
  • At least one of the following appropriate labels must be added to this PR to be consumed into the changelog:
    • Defect: This pull request repairs a github defect issue. The github issue should be referenced in the PR description
    • Refactoring: This pull request includes code changes that don't change the functionality of the program, just perform refactoring
    • NewFeature: This pull request includes code to add a new feature to EnergyPlus
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus
    • DoNoPublish: This pull request includes changes that shouldn't be included in the changelog

Review Checklist

This will not be exhaustively relevant to every PR.

  • Code style (parentheses padding, variable names)
  • Functional code review (it has to work!)
  • If defect, results of running current develop vs this branch should exhibit the fix
  • CI status: all green or justified
  • Performance: CI Linux results include performance check -- verify this
  • Unit Test(s)
  • C++ checks:
    • Argument types
    • If any virtual classes, ensure virtual destructor included, other things
  • IDD changes:
    • Verify naming conventions and styles, memos and notes and defaults
    • If transition, add rules to spreadsheet
    • If transition, add transition source
    • If transition, update idfs
  • If new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • ExpandObjects changes??
  • If output changes, including tabular output structure, add to output rules file for interfaces

…how/where to put this out onto the eio file.

Autocalculate DOAS inputs in Sizing:Zone. Note I haven't figured out …
…how/where to put this out onto the eio file.
@nrel-bot-3

This comment has been minimized.

Copy link

commented on 0090593 Mar 17, 2016

Issue5491 (wfbuhl) - x86_64-MacOS-10.9-clang: OK (2120 of 2129 tests passed, 20 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 11 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 9 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 7 tests had: Table big diffs.
  • 4 tests had: ESO small diffs.
  • 4 tests had: MTR small diffs.
  • 1 test had: Table small diffs.
  • 4 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 17, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: OK (2158 of 2167 tests passed, 20 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 11 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 9 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 7 tests had: Table big diffs.
  • 4 tests had: ESO small diffs.
  • 3 tests had: MTR small diffs.
  • 1 test had: Table small diffs.
  • 4 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1021 of 1021 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1579 of 1579 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - i386-Windows-7-VisualStudio-12: OK (2126 of 2135 tests passed, 20 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 11 tests had: EIO diffs.
  • 9 tests had: ESO big diffs.
  • 7 tests had: Table big diffs.
  • 6 tests had: ERR diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 5 tests had: MTR small diffs.
  • 4 tests had: ESO small diffs.
  • 1 test had: Table small diffs.
  • 4 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 18, 2016

Issue5491 (wfbuhl) - Win64-Windows-7-VisualStudio-12: OK (2126 of 2135 tests passed, 20 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 11 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 9 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 7 tests had: Table big diffs.
  • 4 tests had: ESO small diffs.
  • 4 tests had: MTR small diffs.
  • 1 test had: Table small diffs.
  • 4 tests had: MTR big diffs.

Build Badge Test Badge

Added eio output for Sizing:Zone DOAS inputs when "Account for Dedica…
…ted Outside Air System" = Yes. No SQL output for this yet. Otherwise this is ready to go.
@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented on 4123126 Mar 21, 2016

This needs a unit test. As for SQL and tabular reports, there is no current echo of the Sizing:Zone inputs. I don't feel competent to add it to SQL.

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-MacOS-10.9-clang: Tests Failed (1865 of 2129 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 261 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: Tests Failed (1901 of 2167 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 4 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1021 of 1021 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1579 of 1579 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - i386-Windows-7-VisualStudio-12: Tests Failed (1869 of 2135 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 6 tests had: MTR small diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - Win64-Windows-7-VisualStudio-12: Tests Failed (1869 of 2135 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 6 tests had: MTR big diffs.
  • 8 tests had: ESO small diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

@wfbuhl wfbuhl self-assigned this Mar 21, 2016

@wfbuhl wfbuhl added this to the EnergyPlus 8.5.0 milestone Mar 21, 2016

@wfbuhl wfbuhl assigned mjwitte and wfbuhl and unassigned wfbuhl and mjwitte Mar 21, 2016

@wfbuhl wfbuhl added the Defect label Mar 21, 2016

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented on 4123126 Mar 21, 2016

This needs a unit test. As for SQL and tabular reports, there is no current echo of the Sizing:Zone inputs. I don't feel competent to add it to SQL.

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-MacOS-10.9-clang: Tests Failed (1865 of 2129 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 261 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: Tests Failed (1901 of 2167 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 4 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1021 of 1021 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1579 of 1579 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - i386-Windows-7-VisualStudio-12: Tests Failed (1869 of 2135 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 6 tests had: MTR small diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - Win64-Windows-7-VisualStudio-12: Tests Failed (1869 of 2135 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 6 tests had: MTR big diffs.
  • 8 tests had: ESO small diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

@nrel-bot-3

This comment has been minimized.

Copy link

commented on 9444180 Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-MacOS-10.9-clang: Tests Failed (1866 of 2130 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 261 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1022 of 1022 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: Tests Failed (1902 of 2168 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 4 tests had: MTR small diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 21, 2016

Issue5491 (wfbuhl) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1580 of 1580 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - i386-Windows-7-VisualStudio-12: Tests Failed (1870 of 2136 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 6 tests had: MTR small diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (wfbuhl) - Win64-Windows-7-VisualStudio-12: Tests Failed (1870 of 2136 tests passed, 98 test warnings)

  • 18 tests had: AUD diffs.
  • 11 tests had: RDD diffs.
  • 263 tests had: Table big diffs.
  • 96 tests had: Table small diffs.
  • 25 tests had: EIO diffs.
  • 6 tests had: ERR diffs.
  • 18 tests had: ESO big diffs.
  • 2 tests had: MDD diffs.
  • 2 tests had: MTD diffs.
  • 8 tests had: ESO small diffs.
  • 6 tests had: MTR big diffs.
  • 5 tests had: MTR small diffs.

Build Badge Test Badge

// na

// SUBROUTINE LOCAL VARIABLE DECLARATIONS:
static bool MyOneTimeFlag( true );

This comment has been minimized.

Copy link
@Myoldmopar

Myoldmopar Mar 22, 2016

Member

@wfbuhl So these static variables inside functions cause issues with unit tests. Basically, in unit tests, there is no way to go back into the function and reset the value, so the effects can fall downstream to the next unit test that is run in the same session.

I would:

  • rename this variable to something clear for this application,
  • put it in the anonymous namespace at the top of this file (Fortran-analogy: make it a module variable), and
  • make sure the value gets reset inside the clear_state function already defined for this namespace so the value is reset for any following unit tests that get run.

This comment has been minimized.

Copy link
@wfbuhl

wfbuhl Mar 22, 2016

Author Contributor

Good ideas. Of course this is just a modified copy of ReportSizingOutput.
So similar issues are scattered through the code.

On Tue, Mar 22, 2016 at 8:27 AM, Edwin Lee notifications@github.com wrote:

In src/EnergyPlus/ZoneEquipmentManager.cc
#5557 (comment):

  •   using DataStringGlobals::VerString;
    
  •   using General::RoundSigDigits;
    
  •   // Locals
    
  •   // SUBROUTINE ARGUMENT DEFINITIONS:
    
  •   // SUBROUTINE PARAMETER DEFINITIONS:
    
  •   // INTERFACE BLOCK SPECIFICATIONS
    
  •   // na
    
  •   // DERIVED TYPE DEFINITIONS
    
  •   // na
    
  •   // SUBROUTINE LOCAL VARIABLE DECLARATIONS:
    
  •   static bool MyOneTimeFlag( true );
    

@wfbuhl https://github.com/wfbuhl So these static variables inside
functions cause issues with unit tests. Basically, in unit tests, there is
no way to go back into the function and reset the value, so the effects can
fall downstream to the next unit test that is run in the same session.

I would:


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/NREL/EnergyPlus/pull/5557/files/94441801dcd7780ba27759c138f50bcc360e1792#r57006747

@@ -5455,6 +5459,164 @@ namespace ZoneEquipmentManager {
MassConservation(ZoneNum).MixingSourceMassFlowRate = ZoneSourceMassFlowRate;
}

void

This comment has been minimized.

Copy link
@Myoldmopar

Myoldmopar Mar 22, 2016

Member

This is a nice, packaged up, routine with a clear purpose. Easy to unit test. This is good. I have only three comments, all minor, and more just comments for future coding than this branch.

  • For some reason, the function name indentation is 2 tabs deep, not sure why. If you happen to go back into the code for any changes, just take a tab out and it will fall in line with the rest of the code.
  • I would take out the unused portions of the function comment header. It doesn't hurt that they are there, and since you have included a note in the REFERENCES section, I'd leave that. Just the unused na ones dont need to be included.
  • This would have been ripe for a very tiny move toward object oriented coding. Here's how it would have been done:

ZoneSizingInput is an array of type ZoneSizingInputData. If you go over to that struct definition, you could add a function declaration like this:

struct ZoneSizingInputData {
  // lots of stuff
  // variables, etc.
  void sizeDOASControlStrategy();
  // other stuff
}

Now there would be a function called sizeDOASControlStrategy that is a member of the struct. To call this function, you would then loop over the instances and call that method:

for ( auto & thisZoneData : ZoneSizingInput ) {
  thisZoneData.sizeDOASControlStrategy();
}

So far so good. Then inside sizeDOASControlStrategy, you would just operate on the current instance:

void ZoneSizingInputData::sizeDOASControlStrategy() {
  // comments
  if ( this->AccountForDOAS ) {
    if ( this->DOASControlStrategy == DOASNeutralSup ) {
      if ( this->DOASLowSetpoint == AutoSize && this->DOASHighSetpoint == AutoSize ) {
        this->DOASLowSetpoint = 21.1;
        this->DOASHighSetpoint = 23.9;
      }
  // etc...

Inside that function, you are always operating on the "current zone sizing input", or "THIS zone sizing input". That's the purpose of the this->, which is actually optional.

This would be a small change, but would be one step closer to objectification, and even easier to unit test, in my opinion. And it's also a form of "gateway" into OO since it is such an isolated change.

OK, that is all, thanks.

This comment has been minimized.

Copy link
@wfbuhl

wfbuhl Mar 22, 2016

Author Contributor

This is modest enough that it can squeeze into my unused brain cells.

On Tue, Mar 22, 2016 at 8:40 AM, Edwin Lee notifications@github.com wrote:

In src/EnergyPlus/ZoneEquipmentManager.cc
#5557 (comment):

@@ -5455,6 +5459,164 @@ namespace ZoneEquipmentManager {
MassConservation(ZoneNum).MixingSourceMassFlowRate = ZoneSourceMassFlowRate;
}

  • void

This is a nice, packaged up, routine with a clear purpose. Easy to unit
test. This is good. I have only three comments, all minor, and more just
comments for future coding than this branch.

  • For some reason, the function name indentation is 2 tabs deep, not
    sure why. If you happen to go back into the code for any changes, just take
    a tab out and it will fall in line with the rest of the code.
  • I would take out the unused portions of the function comment header.
    It doesn't hurt that they are there, and since you have included a note in
    the REFERENCES section, I'd leave that. Just the unused na ones dont need
    to be included.
  • This would have been ripe for a very tiny move toward object
    oriented coding. Here's how it would have been done:

ZoneSizingInput is an array of type ZoneSizingInputData. If you go over
to that struct definition, you could add a function declaration like this:

struct ZoneSizingInputData {
// lots of stuff
// variables, etc.
void sizeDOASControlStrategy();
// other stuff
}

Now there would be a function called sizeDOASControlStrategy that is a
member of the struct. To call this function, you would then loop over
the instances and call that method:

for ( auto & thisZoneData : ZoneSizingInput ) {
thisZoneData.sizeDOASControlStrategy();
}

So far so good. Then inside sizeDOASControlStrategy, you would just
operate on the current instance:

void ZoneSizingInputData::sizeDOASControlStrategy() {
// comments
if ( this->AccountForDOAS ) {
if ( this->DOASControlStrategy == DOASNeutralSup ) {
if ( this->DOASLowSetpoint == AutoSize && this->DOASHighSetpoint == AutoSize ) {
this->DOASLowSetpoint = 21.1;
this->DOASHighSetpoint = 23.9;
}
// etc...

Inside that function, you are always operating on the "current zone sizing
input", or "THIS zone sizing input". That's the purpose of the this->,
which is actually optional.

This would be a small change, but would be one step closer to
objectification, and even easier to unit test, in my opinion. And it's also
a form of "gateway" into OO since it is such an isolated change.

OK, that is all, thanks.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/NREL/EnergyPlus/pull/5557/files/94441801dcd7780ba27759c138f50bcc360e1792#r57009446

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

So do you anticipate doing SQL output for this? Or are you saying this will have to wait for later and you want to wrap this up as-is?

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

(Saw your email @wfbuhl, will communicate there)

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

OK, I merged in develop, cleaned up the indentation issues, and moved that variable out of the function and added it to the clear_state function. Since we won't be holding this up for the SQL output portion, this should be good to go as long as CI comes back happy.

@nrel-bot-2

This comment has been minimized.

Copy link

commented on 5777495 Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-Linux-Ubuntu-14.04-gcc-4.8: OK (2203 of 2203 tests passed, 3 test warnings)

  • 3 tests had: EIO diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-Linux-Ubuntu-14.04-cppcheck-1.61: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-MacOS-10.9-clang: OK (2165 of 2165 tests passed, 3 test warnings)

  • 3 tests had: EIO diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug: OK (1057 of 1057 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-Linux-Ubuntu-14.04-custom_check: OK (0 of 0 tests passed, 0 test warnings)

Build Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - Win64-Windows-7-VisualStudio-12: OK (2171 of 2171 tests passed, 3 test warnings)

  • 3 tests had: EIO diffs.

Build Badge Test Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug: OK (1615 of 1615 tests passed, 0 test warnings)

Build Badge Test Badge Coverage Badge

This comment has been minimized.

Copy link

replied Mar 22, 2016

Issue5491 (Myoldmopar) - i386-Windows-7-VisualStudio-12: OK (2171 of 2171 tests passed, 3 test warnings)

  • 3 tests had: EIO diffs.

Build Badge Test Badge

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

Everything looks hunky dory; the only diffs being the eio line additions, so this should drop right in.

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

Make a new issue on verification of sizing inputs. This is getting somewhat
important since sizing input is getting more complicated. Should come out
on tabular and SQL. From my quick scan of the SQL routine, there seems to
be some of it hard-coded and some that is more flexible. The thing that
baffled me was how to assign labels or tags to new data. I'm sure I could
figure it out if given a little time. Tabular I've done before. But it
didn't seem worth the effort to just do this for DOAS sizing. The whole
shebang needs to be output to tabular and SQL Also I haven't looked into
how to test SQL.

On Tue, Mar 22, 2016 at 8:41 AM, Edwin Lee notifications@github.com wrote:

So do you anticipate doing SQL output for this? Or are you saying this
will have to wait for later and you want to wrap this up as-is?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5557 (comment)

@Myoldmopar

This comment has been minimized.

Copy link
Member

commented Mar 22, 2016

Make a new issue on verification of sizing inputs.

Are you saying you are doing it? Or was that a command? Because if it was a command, you're going to need to prefix with sudo.

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

Should only change the one eio file.

On Tue, Mar 22, 2016 at 9:04 AM, Edwin Lee notifications@github.com wrote:

OK, I merged in develop, cleaned up the indentation issues, and moved that
variable out of the function and added it to the clear_state function.
Since we won't be holding this up for the SQL output portion, this should
be good to go as long as CI comes back happy.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5557 (comment)

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

Well now I don't have anything to do today.

On Tue, Mar 22, 2016 at 9:04 AM, Edwin Lee notifications@github.com wrote:

OK, I merged in develop, cleaned up the indentation issues, and moved that
variable out of the function and added it to the clear_state function.
Since we won't be holding this up for the SQL output portion, this should
be good to go as long as CI comes back happy.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5557 (comment)

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

See previous.

On Tue, Mar 22, 2016 at 8:46 AM, Edwin Lee notifications@github.com wrote:

(Saw your email @wfbuhl https://github.com/wfbuhl, will communicate
there)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5557 (comment)

@wfbuhl

This comment has been minimized.

Copy link
Contributor Author

commented Mar 22, 2016

I will do it since my day has suddenly opened up.

On Tue, Mar 22, 2016 at 10:47 AM, Edwin Lee notifications@github.com
wrote:

Make a new issue on verification of sizing inputs.

Are you saying you are doing it? Or was that a command? Because if it was
a command, you're going to need to prefix with sudo.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5557 (comment)

Myoldmopar added a commit that referenced this pull request Mar 22, 2016

Merge pull request #5557 from NREL/Issue5491
Autocalculate DOAS inputs in Sizing:Zone. Note I haven't figured out …

@Myoldmopar Myoldmopar merged commit cfe9e4d into develop Mar 22, 2016

8 checks passed

Win64-Windows-7-VisualStudio-12 OK (2171 of 2171 tests passed, 3 test warnings)
Details
i386-Windows-7-VisualStudio-12 OK (2171 of 2171 tests passed, 3 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-cppcheck-1.61 OK (0 of 0 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-custom_check OK (0 of 0 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8 OK (2203 of 2203 tests passed, 3 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8-IntegrationCoverage-Debug OK (1615 of 1615 tests passed, 0 test warnings)
Details
x86_64-Linux-Ubuntu-14.04-gcc-4.8-UnitTestsCoverage-Debug OK (1057 of 1057 tests passed, 0 test warnings)
Details
x86_64-MacOS-10.9-clang OK (2165 of 2165 tests passed, 3 test warnings)
Details

@Myoldmopar Myoldmopar deleted the Issue5491 branch Mar 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.