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.

Sign up for GitHub

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

Jump to bottom

Add Transition Rules for EMS @CpAirFnWTdb to @CpAirFnW #7801

Merged
merged 4 commits into from
Feb 22, 2020
Merged

Add Transition Rules for EMS @CpAirFnWTdb to @CpAirFnW #7801

merged 4 commits into from
Feb 22, 2020

Conversation

mitchute
Copy link
Collaborator

This adds transition rules for changing the EMS function @CpAirFnWTdb to @CpAirFnW.

EMS Program/Subroutine Usage Summary

Previous: SET cp_air = @CpAirFnWTdb HumRate TempDB
Current: SET cp_air = @CpAirFnW HumRate

Previous PRs which are completed but related to this transition are #7479 and #7755.

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

@mitchute mitchute added the DoNotPublish Includes changes that shouldn't be reported in the changelog label Feb 21, 2020
@mitchute mitchute added this to the EnergyPlus 9.3.0 milestone Feb 21, 2020
@mitchute mitchute self-assigned this Feb 21, 2020
@mitchute mitchute requested a review from Nigusse February 21, 2020 21:13
@mjwitte
Copy link
Contributor

mjwitte commented Feb 21, 2020 •
edited
Loading

@Myoldmopar While it's not an IDD change, this should go in before I/O Freeze.

mjwitte
mjwitte reviewed Feb 21, 2020
View reviewed changes
Copy link
Contributor

@mjwitte mjwitte left a comment

Choose a reason for hiding this comment

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

This needs an entry in the input rules doc as well.

src/Transition/CreateNewIDFUsingRulesV9_3_0.f90
END SUBROUTINE SortUnique

SUBROUTINE UpdateEMSFunctionName(InOutArg, OldName, NewName, NoDiffArg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow - this got overly complicated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 Getting paid by the line over here!

I'm happy to make changes, though, but I might need some suggestions from a Fortran expert.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it ain't broke . . . I'll test it shortly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mitchute I do not have Fortran compiler in my machine, so I cannot make complete review. I kindly request @mjwitte to review. Thanks.

@Myoldmopar
Copy link
Member

Built locally and got good results on four example files with this function. Comparing the 9.2 versions of the files with the newly transitioned versions, and grepping to only see the CpAir diffs:

$ diff 1ZoneDataCenterCRAC_wApproachTemp.idfold 1ZoneDataCenterCRAC_wApproachTemp.idfnew | grep CpAir
<     set cpair = @CpAirFnWTdb Wout_SupplyFan  Tout_SupplyFan,  !- Program Line 1
>     set cpair = @CpAirFnW Wout_SupplyFan,  !- Program Line 1

$ diff 1ZoneDataCenterCRAC_wPumpedDXCoolingCoil.idfold 1ZoneDataCenterCRAC_wPumpedDXCoolingCoil.idfnew | grep CpAir
<     set cpair = @CpAirFnWTdb Wout_SupplyFan  Tout_SupplyFan,  !- Program Line 1
>     set cpair = @CpAirFnW Wout_SupplyFan,  !- Program Line 1

$ diff EMSUserDefined5ZoneAirCooled.idfold EMSUserDefined5ZoneAirCooled.idfnew | grep CpAir
<     Set HeatingAir_CP     = @CpAirFnWTdb HeatingZoneW AvgSAT,  !- <none>
>     Set HeatingAir_CP = @CpAirFnW HeatingZoneW,  !- <none>
<     Set HeatingAir_CP     = @CpAirFnWTdb HeatingZoneW AvgSAT,  !- <none>
>     Set HeatingAir_CP = @CpAirFnW HeatingZoneW,  !- <none>
<     Set HeatingAir_CP     = @CpAirFnWTdb HeatingZoneW AvgSAT,  !- <none>
>     Set HeatingAir_CP = @CpAirFnW HeatingZoneW,  !- <none>
<     Set HeatingAir_CP     = @CpAirFnWTdb HeatingZoneW AvgSAT,  !- <none>
>     Set HeatingAir_CP = @CpAirFnW HeatingZoneW,  !- <none>
<     Set HeatingAir_CP     = @CpAirFnWTdb HeatingZoneW AvgSAT,  !- <none>
>     Set HeatingAir_CP = @CpAirFnW HeatingZoneW,  !- <none>

$ diff EMSUserDefinedWindACAuto.idfold EMSUserDefinedWindACAuto.idfnew | grep CpAir
<     Set Mix_Cp = @CpAirFnWTdb Mix_W Mix_Tdb,  !- <none>
>     Set Mix_Cp = @CpAirFnW Mix_W,  !- <none>
[edwin@thebeast] [16:01:53] [/eplus/repos/2eplus/builds/r/Products] [wall: 0s]
$

@mitchute
Copy link
Collaborator Author

@mjwitte input rules notes added here: 0ae0991

Myoldmopar
Myoldmopar approved these changes Feb 21, 2020
View reviewed changes
src/Transition/CreateNewIDFUsingRulesV9_3_0.f90

IF (CurArgs .GE. 2) THEN
DO I = 2, CurArgs, 1
CALL UpdateEMSFunctionName(OutArgs(I), EMSOldName, EMSNewName, NoDiff)
Copy link
Member

Choose a reason for hiding this comment

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

Good job capturing this in a separate utility function.

src/Transition/CreateNewIDFUsingRulesV9_3_0.f90

PARAMETER(N=20)
CHARACTER*20 ARRAY(N)
Copy link
Member

Choose a reason for hiding this comment

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

So this will allow up to 20 tokens to come back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's just a guess at a conservative number.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I mean given the line length limit that was put on IDFs, this seems reasonable.

src/Transition/CreateNewIDFUsingRulesV9_3_0.f90
NewInOutArg(IDXStart:IDXEnd) = ARRAY(J)
IDXStart = IDXEnd + 2
END DO
Copy link
Member

Choose a reason for hiding this comment

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

This is gross. Why not just ' '.join(newArray)?

Looks good. 🙃

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make it happen!

@mjwitte
Copy link
Contributor

mjwitte commented Feb 21, 2020

@mitchute @Nigusse If you run 1ZoneDataCenterCRAC_wApproachTemp (transitioned up from 9.2 without this new rule) with the old function, the error message is a bit confusing:

   ** Severe  ** Problem found in EMS EnergyPlus Runtime Language.
   **   ~~~   ** Erl program name: CALCSCOP
   **   ~~~   ** Erl program line number: 1
   **   ~~~   ** Erl program line text: SET CPAIR = @CPAIRFNWTDB WOUT_SUPPLYFAN  TOUT_SUPPLYFAN
   **   ~~~   ** Error message:  *** Error: EvaluateExpression: Variable = 'TDB' used in expression has not been initialized! ***

Seems the EMS input should trap CPAIRFNWTDB as an illegal function. This can be fixed after I/O freeze, but would be good to clean up.

@mitchute
Copy link
Collaborator Author

@mjwitte Good catch. I'll make a note to clean that up.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 21, 2020 •
edited
Loading

Hmm, testing this on Windows with 1ZoneDataCenterCRAC_wApproachTemp, I get:

    set cpair = @CpAirFnW Wout_SupplyFan�   Œ   �   ì���£yR �`O ì���£yR �   ï   èü���   8çz ö�  ðo���   ,  !- Program Line 1

@mjwitte
Copy link
Contributor

mjwitte commented Feb 21, 2020

May need a TRIM() somewhere.

Myoldmopar
Myoldmopar reviewed Feb 22, 2020
View reviewed changes
src/Transition/CreateNewIDFUsingRulesV9_3_0.f90
LenToken = LEN_TRIM(ADJUSTL(ARRAY(J)))
IDXEnd = LenToken + IDXStart - 1
NewInOutArg(IDXStart:IDXEnd) = ARRAY(J)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this may need to be TRIM(ADJUSTL(ARRAY(J)))?. I'm not sure. I know different platforms might initialize data differently, but I'm surprised this specific thing would work on a platform and not the other.

@Myoldmopar
Copy link
Member

I was able to reproduce the problem on Windows, and I was able to find the solution and properly transition on Windows. I am going to merge this.

@Myoldmopar Myoldmopar merged commit cc126b8 into develop Feb 22, 2020
@Myoldmopar Myoldmopar deleted the transition_ems_func branch February 22, 2020 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjwitte mjwitte mjwitte left review comments

@Myoldmopar Myoldmopar Myoldmopar approved these changes

@Nigusse Nigusse Awaiting requested review from Nigusse

Assignees

@mitchute mitchute

Labels
DoNotPublish Includes changes that shouldn't be reported in the changelog
Projects
None yet
Milestone
EnergyPlus 9.3.0
Development

Successfully merging this pull request may close these issues.
8 participants
@mitchute @mjwitte @Myoldmopar @Nigusse @nrel-bot-2 @nrel-bot-3 @nrel-bot-2b @nrel-bot-2c