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

PR #8997 breaks transitions for Wall:Detailed, etc for version < 9.6 #9172

Closed
1 of 3 tasks
hongyuanjia opened this issue Nov 7, 2021 · 3 comments
Closed
1 of 3 tasks

Comments

@hongyuanjia
Copy link

hongyuanjia commented Nov 7, 2021

Issue overview

PR #8997 introduced some changes in the VCompareGlobalRoutines.f90 for compatibility with the Space def introduced. However, those changes break transitions that involve detailed geometries, e.g. Wall:Detailed, Floor:Detailed, including transitions from v7.2 to v8.0, and v8.7 to v8.8.

I maintain an R package eplusr which implements pure R-based transition programs. Tests are run to make sure the results are the same between R and EnergyPlus Transition programs. The CI gives me errors after I try to use EnergyPlus v9.6.

══ Failed tests ════════════════════════════════════════════════════════════════
── Failure (test-transition.R:199:5): Transition v7.2 --> v8.0 ─────────────────
idfVU$"Wall:Detailed"$Wall$value() not equal to idfTR$"Wall:Detailed"$Wall$value().
Length mismatch: comparison on first 39 components
── Failure (test-transition.R:204:5): Transition v7.2 --> v8.0 ─────────────────
idfVU$"RoofCeiling:Detailed"$Roof$value() not equal to idfTR$"RoofCeiling:Detailed"$Roof$value().
Length mismatch: comparison on first 39 components
── Failure (test-transition.R:209:5): Transition v7.2 --> v8.0 ─────────────────
idfVU$"Floor:Detailed"$Floor$value() not equal to idfTR$"Floor:Detailed"$Floor$value().
Length mismatch: comparison on first 39 components
── Failure (test-transition.R:1252:5): Transition v8.7 --> v8.8 ────────────────
idfVU$"Floor:Detailed"$Surf2$value() not equal to idfTR$"Floor:Detailed"$Surf2$value(1:21).
Length mismatch: comparison on first 21 components
Component "Vertex 1 Y-coordinate": Mean relative difference: 0.3777778
Component "Vertex 1 Z-coordinate": Mean relative difference: 1
Component "Vertex 2 X-coordinate": Mean absolute difference: 45
Component "Vertex 2 Y-coordinate": Mean relative difference: 0.9111111
Component "Vertex 2 Z-coordinate": Mean relative difference: 1
Component "Vertex 3 X-coordinate": Mean absolute difference: 4
Component "Vertex 3 Z-coordinate": Mean relative difference: 1
Component "Vertex 4 X-coordinate": Mean absolute difference: 4
...

Take the floor Zn001:Flr001 as an example:

  Floor:Detailed,
    Zn001:Flr001,            !- Name
    FLOOR,                   !- Construction Name
    ZONE ONE,                !- Zone Name
    Adiabatic,               !- Outside Boundary Condition
    ,                        !- Outside Boundary Condition Object
    NoSun,                   !- Sun Exposure
    NoWind,                  !- Wind Exposure
    1.000000,                !- View Factor to Ground
    4,                       !- Number of Vertices
    15.24000,0.000000,0.0,  !- X,Y,Z ==> Vertex 1 {m}
    0.000000,0.000000,0.0,  !- X,Y,Z ==> Vertex 2 {m}
    0.000000,15.24000,0.0,  !- X,Y,Z ==> Vertex 3 {m}
    15.24000,15.24000,0.0;  !- X,Y,Z ==> Vertex 4 {m}

After transitioning from v8.7 to v8.8 using VersionUpdater from EnergyPlus v9.6, the results are below. Note that the value of Vertex 1 X-coordinate has been duplicated.

  Floor:Detailed,
    Zn001:Flr001,            !- Name
    FLOOR,                   !- Construction Name
    ZONE ONE,                !- Zone Name
    Adiabatic,               !- Outside Boundary Condition
    ,                        !- Outside Boundary Condition Object
    NoSun,                   !- Sun Exposure
    NoWind,                  !- Wind Exposure
    1.000000,                !- View Factor to Ground
    4,                       !- Number of Vertices
    15.24000,                !- Vertex 1 X-coordinate {m}
    15.24000,0.000000,0.0,  !- X,Y,Z ==> Vertex 1 {m}
    0.000000,0.000000,0.0,  !- X,Y,Z ==> Vertex 2 {m}
    0.000000,15.24000,0.0,  !- X,Y,Z ==> Vertex 3 {m}
    15.24000,15.24000,0.0;  !- X,Y,Z ==> Vertex 4 {m}

The error comes from the wrong number of fields given in WriteOutPartialIDFLines after parsing the field Number of Vertices.

CASE('WALL:DETAILED','ROOFCEILING:DETAILED','FLOOR:DETAILED')
NVertFieldNum = 9 + SurfaceSpaceFieldShift
IF (MakeUPPERCase(OutArgs(NVertFieldNum)) == 'AUTOCALCULATE') THEN
NVert=(CurArgs-NVertFieldNum)/3
ELSEIF (OutArgs(NVertFieldNum) == '') THEN
NVert=(CurArgs-NVertFieldNum)/3
ELSE
! Try to read the number of vertices as an integer
NVert = ProcessNumber(OutArgs(NVertFieldNum),ErrFlag)
IF (ErrFlag) THEN
! If failed, default to autocalculate instead
NVert=(CurArgs-NVertFieldNum)/3
! Replace in existing with Autocalculate
OutArgs(NVertFieldNum) = 'Autocalculate'
CALL ShowWarningError('For ' // TRIM(ObjectName) // ' named ''' // TRIM(OutArgs(1)) // &
''', Number of vertices is not a number, defaulting to Autocalculate (N='// &
TRIM(TrimSigDigits(NVert)) //')',Auditf)
ELSE
! Silently trim any float to an integer
OutArgs(NVertFieldNum) = TrimSigDigits(NVert)
ENDIF
ENDIF
! Now we write the first few fields
CALL WriteOutPartialIDFLines(DifUnit,ObjectName,10,OutArgs,FieldNames,FieldUnits)

Instead of giving a fixed 10, the number of fields to be written should be based on NvertFiledNum. Just as what has been done to the case for BuildingSurface:Detailed. So L971 should be changed to below:

CALL WriteOutPartialIDFLines(DifUnit,ObjectName, **NVertFieldNum**,OutArgs,FieldNames,FieldUnits)

Details

Some additional details for this issue (if relevant):

  • Platform (Operating system, version): All
  • Version of EnergyPlus: Offical v9.6
  • Unmethours link or helpdesk ticket number

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Defect file added: 1ZoneUncontrolled_FoorDetailed.idf.txt
  • Ticket added to Pivotal for defect (development team task)
  • Pull request created (the pull request will have additional tasks related to reviewing changes that fix this defect)
hongyuanjia added a commit to hongyuanjia/eplusr that referenced this issue Nov 7, 2021
hongyuanjia added a commit to hongyuanjia/eplusr that referenced this issue Nov 7, 2021
* [feat] Update version and internal data

* [fix] Update transition warning class

* [feat] Add v9.5 to v9.6 transition fun

* [test] Fix tests

* [doc] Bump dev version and update NEWS

* [doc] Update latest version in README

* [fix] Only install EnergyPlus v9.6 when testing the corresponding transition

See NREL/EnergyPlus#9172
@mjwitte
Copy link
Contributor

mjwitte commented Nov 8, 2021

@jcyuan2020 Please add this fix to #9139.

@jcyuan2020
Copy link
Contributor

This one is added too. Thanks @hongyuanjia @mjwitte

@mjwitte
Copy link
Contributor

mjwitte commented Jan 19, 2022

Addressed in #9139

@mjwitte mjwitte closed this as completed Jan 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants