Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fixed slat angle control and slat angle reporting issues for VB in equivalent layer window model #6191

Merged
merged 14 commits into from
Jul 29, 2017

Conversation

Nigusse
Copy link
Contributor

@Nigusse Nigusse commented Jun 30, 2017

Issue #5750

Pull request overview

This is a fix for defects in Equivalent Layer Window Model Venetian Blind (VB) slat angle control and slat angle reporting issues. Now the slat angle control works as expected and the slat angle are reported for the three control modes. Also fixed the slat angle calculation for BlockBeamSolar control mode. Diffs are expected for BlockBeamSolar Slat Angle Control. Also swapped vertical and horizontal profile angles for equivalent layer window model.

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
    • Performance: This pull request includes code changes that are directed at improving the runtime performance of EnergyPlus

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 new idf included, locally check the err file and other outputs
  • Documentation changes in place
  • Changed docs build successfully
  • If output changes, including tabular output structure, add to output rules file for interfaces

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 7, 2017

The defect has been fixed and ready for review.

@mjwitte mjwitte added Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze) labels Jul 18, 2017
@Myoldmopar
Copy link
Member

Yikes. I tried resolving the conflict with the online editor. Looks like it was trying to fix our whitespace for us, inserting spaces. It looks like the conflict is just that two branches added some using statements. No big deal. I can fix that and push this back up. I'll make sure it is updated with develop also.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

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

@Nigusse This looks pretty good. This will need a transition rule to change the user's angle input right? Or was it just being misinterpreted and we need to leave their input alone?

Other minor comments that will get cleaned up if transition rules or other code changes are needed.

// issue #5750, commented out because it does not block the solar beam
//RAT = L.S * std::cos( OMEGA_DEG ) / L.W;
//// limit upward slat angle to horiz = max visibility
//VB_CriticalSlatAngle = max( 0.0, RadiansToDeg * std::asin( RAT ) - OMEGA_DEG );
Copy link
Member

Choose a reason for hiding this comment

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

Should just clean these out. With our version control we can always look back and see where the file was modified, and with good commit messages we can see why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. I will clean that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cleaned commented code lines.

@@ -9259,16 +9267,16 @@ namespace WindowEquivalentLayer {
ProfAngVer = 0.0;
ConstrNum = Surface( SurfNum ).Construction;
EQLNum = Construct( Surface( SurfNum ).Construction ).EQLConsPtr;

Copy link
Member

Choose a reason for hiding this comment

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

Stray whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed white space.

\default 45
\note Slat angle is +ve if the tip of the slat front face is tilted upward, else
\note the slat angle is -ve if the tip of the slat front face is tilted downward.
\note The slat angle varies between -90 to +90. The default value is 45 degrees.
Copy link
Member

Choose a reason for hiding this comment

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

OK. So this needs a transition rule to subtract 90 degrees from the original input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably Slat Angle > 0 should remain the same but if greater than 90 then 90 minus Slat Angle should apply. I will double check this transition rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added transition rule (both *.md and *.f90 files).

Copy link
Member

Choose a reason for hiding this comment

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

Great. I'll check it out once you get the changes pushed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I run into merge conflicts and had to commit twice.

ObjectName='WindowMaterial:Blind:EquivalentLayer'
CALL GetNewObjectDefInIDD(ObjectName,NwNumArgs,NwAorN,NwReqFld,NwObjMinFlds,NwFldNames,NwFldDefaults,NwFldUnits)
OutArgs(1:24) = InArgs(1:24)
IF (inArgs(6) >= '90') THEN
Copy link
Member

Choose a reason for hiding this comment

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

Umm. Does this actually compile and run?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. I never run transition codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will push a corrected version.

@Myoldmopar
Copy link
Member

@Nigusse Hey, thanks for going ahead and adding the transition rule. I was happy to do it, but thanks for forging ahead on it.

I am just wanting to verify the transition. If before it went from 0-180, and now it goes from -90 to 90, why aren't you just shifting the value down by 90? Maybe I don't fully grok the way in which the original code was broken, so I don't see why you are only transitioning this way. If it is right, can you can clarify it one more time, and if all is good we'll merge this in. (I'll test the transition locally now.)

@mjwitte
Copy link
Contributor

mjwitte commented Jul 28, 2017

Who added the equivalent layer model? If we change the equivalent layer blinds to be -90 to +90 while regular blinds are 0 to 180 that seems a bit confusing.

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 28, 2017

@Myoldmopar @mjwitte The original equivalent window (ASHWAT) model uses the +90/-90 convention while EPlus uses 0 - 180 convention. So, the equivalent window blind model is based on +90/-90 convention but the idd was wrong (adopted Eplu's convention). @mjwitte I added the equivalent window model to Eplus but it was added in hasty situation when the original contractors did not deliver.

@Nigusse
Copy link
Contributor Author

Nigusse commented Jul 28, 2017

@Myoldmopar Just to clarify the slat angle convention in equivalent window model did not change only the idd changed to match the model in the code. So, from functional point of view the model was already using +90/-90 convention.

@Myoldmopar
Copy link
Member

OK, so do we really not have an example file for this object? I find several WindowMaterial:Blind, but not :EquivalentLayer. OK, well, I took the defect file, which was in 8.5, transitioned it up to 8.7 and then verified the transition works as expected. If I use an angle < 90, it leaves it. Anything over it and it subtracts 90. So now the angle works as the code has expected it to work. I think this is done. Windows 32-bit is coming back with 2568 or 2568 tests passing, so that's about as good as I'd expect from CI. I'll merge this.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Defect Includes code to repair a defect in EnergyPlus IDDChange Code changes impact the IDD file (cannot be merged after IO freeze)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants