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

Implement additional chiller part load curve type #4727

Merged
merged 17 commits into from
Feb 21, 2015

Conversation

rongpengzhang
Copy link
Contributor

@Introduction of the feature:
Chillers with variable speed drive (VSD Chillers) on compressors, reset control of entering condenser water temperature and leaving chilled water temperature can operate much more efficiently at part load conditions. An additional part-load performance curve type is proposed for such chillers in EnergyPlus. The new curve type is a modification to the existing curve and can model performance of VSD chillers more accurately.

@summary of the changes:
(1) 2 existing CC files and 2 existing HH files are updated
(2) IDD file is modified
(3) 1 new IDF example file is added
(4) 10 existing IDF example files are updated

rongpengzhang and others added 7 commits February 13, 2015 09:25
Changes include the following:
(1) 1 IDD file
(2) 2 CC files
(3) 2 HH files
Changes include the following:
(1) 1 new IDF file is added
(ChillerPartLoadCurve_RefBldgLargeOfficeNew2004_Chicago.idf)
(2) 10 existing IDF files are modified
Padding issues corrected
Hopefully diff comes up clean this time; this was troublesome
@Myoldmopar
Copy link
Member

OK, @rongpengzhang I pushed up some changes. These include:

  • Updating transition source to accomodate the IDD changes. This was a big burden, and I had to for some reason clean up a bunch of Transition source. Anyway, the idfs are coming out clean and look great.
  • Passing the relevant idfs through Transition to make sure they are clean and only show relevant diffs. There are Sizing:System diffs in the field names only, which I think is because the idfs on develop were slightly out of date and hadn't been passed through Transition (looks like it went in with PR Revised supply air flow rate fields in multiple IDD objects #4642)
  • Merging develop into this branch to ensure we are up to date to avoid any regression issues.

Let's wait and see what the robots say about this last commit (beefc61), and if everything looks good, we should be very close to merging. Can you upload the doc changes (IORef, EngRef) to a new folder in here?

@rongpengzhang
Copy link
Contributor Author

Thanks, @Myoldmopar I can tell the transition updating is tough work. Good job!

I will upload the IORef and EngRef ASAP.

@Myoldmopar
Copy link
Member

@rongpengzhang Verified that the one failure should be cleaned up with the updated reference temperature. This should be good to go with beefb3d and the doc changes over in BuildSupport.

@mjwitte This was proposed and approved a long time ago, so it's not worth a whole lot of scrutiny, but if you wouldn't mind taking a minute and glancing over, for example, the IDD changes, I would appreciate it.

@Myoldmopar
Copy link
Member

@rongpengzhang OK, things are looking good. As expected, the cppcheck, unit test coverage, and integration test coverage all came back fully green. And also as expected, the Ubuntu and Mac builds with regression tests are coming back with one diff, the idf we changed. The WIndows machine is really busy, but I'll give it a little time this morning to get one result in. This will give some more time to get the doc changes uploaded to BuildSupport anyway.

@Myoldmopar
Copy link
Member

Oh, and @rongpengzhang, in your pull request at the top, you @ tagged two random people: introduction and summary. If you want titles, just use # or ## or ###, don't use the @ symbol. (Formatting tips here)

\note Calculated based on dT*, Tdev* and PLR
\note curve = a + b*(dT*) + c*(dT*)**2 + d*PLR + e*PLR**2 + f*(dT*)*PLR + g*(dT*)**3
\note + h*PLR**3 + i*(dT*)**2*PLR + j*(dT*)*PLR**2 + k*(dT*)**2*PLR**2 + l*(Tdev*)*PLR**3
\note Tdev = Leaving Chilled Water Temperature � Reference Chilled Water Temperature
Copy link
Contributor

Choose a reason for hiding this comment

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

@rongpengzhang There is a special character in this line - shows as a box on github.

@rongpengzhang
Copy link
Contributor Author

@Myoldmopar I'm happy that things are going well as what we expected. Thanks for the formatting tips, I didn't know there are better choices to make the title bold ~

@mjwitte Thanks a lot for the detailed IDD check and suggestion!
I will take care of the special character issue.
Yes, "BiVariateTables" should be kept there.
The updated IDD file will be uploaded very soon.

@Myoldmopar
Copy link
Member

@rongpengzhang CI looks great. Once you are done with the IDD changes, I'll update the transition one more time per @mjwitte's comments, along with the idfs. We'll have to let CI have one last pass to make sure the IDD changes don't accidentally mess up anything, but then merge.

(1) IDD files updated
(2) 1 cc file and 1 hh file are updated, due to IDD updates
(3) 1 IDF file updated
@rongpengzhang
Copy link
Contributor Author

@Myoldmopar I have updated the IDD file taking @mjwitte 's comments. Can you let CI to run another test (cc and idf files are also changed due to idd updates)

@mjwitte
Copy link
Contributor

mjwitte commented Feb 19, 2015

@rongpengzhang There's new warnings with HospitalLowEnergy?
@Myoldmopar I'm going to make a few more minor tweaks to the IDD - shouldn't impact any test runs.

if ( lAlphaFieldBlanks( 5 ) ) {

//Check the type of part-load curves implemented: 1_LeavingCondenserWaterTemperature, 2_Lift zrp_Aug2014
if ( SameString( PartLoadCurveType, "LeavingCondenserWaterTemperature" ) && SameString( GetCurveType( ElecReformEIRChiller( EIRChillerNum ).ChillerEIRFPLR ), "BICUBIC" ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rongpengzhang Didn't notice these before. These if blocks need to be expanded to include the valid table types. Some components allow any form a curve as long as it has the same number of independent variables. Do you feel strongly that this should only allow these specific curve forms? e.g. Should biquadratic be illegal, or just a warning, or completely acceptable? @mmAtTs @Myoldmopar @EnergyArchmage Comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mjwitte
It seems that the users are required to use "BICUBIC" , rather than other curve types, to describe this specific curve ("Electric Input to Cooling Output Ratio Function of Part Load Ratio Curve"). Details can be found in the following iDD sections. Please let me know if you think "warning" is better. Of course, table types need to be added if we keep using "severe error"

Chiller:Electric:ReformulatedEIR,
A5 , \field Electric Input to Cooling Output Ratio Function of Part Load Ratio Curve Name
\type object-list
\object-list BicubicCurves
\object-list BiVariateTables
\object-list ChillerPartLoadWithLiftCurves
\note two types of part-load curves are available: BicubicCurves or ChillerPartLoadWithLiftCurves
\note Type 1: BicubicCurves;
\note Calculated based on LCT and PLR
\note curve = a + b_LCT + c_LCT2 + d_PLR + e_PLR2 + f_LCT_PLR + g_0 + h_PLR3
\note + i_0 + j_0
\note PLR = part load ratio (cooling load/steady state capacity)
\note LCT = leaving condenser fluid temperature(C)
\note Type 2: ChillerPartLoadWithLiftCurves
\note Calculated based on dT_, Tdev_ and PLR
\note curve = a + b_(dT_) + c_(dT_)2 + d_PLR + e_PLR2 + f_(dT_)PLR + g(dT*)3
\note + h_PLR__3 + i_(dT_)2_PLR + j(dT)PLR__2 + k(dT_)__2_PLR2 + l_(Tdev_)*PLR
3

Copy link
Member

Choose a reason for hiding this comment

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

I guess since the enforcement is actually on BiCubic curves or tables, and if you enter BiCubic as the table type, this enforcement is fine. Or am I missing something now?

@Myoldmopar
Copy link
Member

@mjwitte No, there are fewer warnings in HospitalLowEnergy. The lines in the error file are now removed.

@mjwitte I'm guessing any two-independent-variable form is allowable, including tables, curves, and any shape. Thus, it should be loosened up.

@rongpengzhang I will update the transition source per @mjwitte and check that in. Then the curve type check in GetInput needs to be resolved. Hopefully that's that last issue.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 19, 2015

@Myoldmopar Whoops - sorry - still not used to the color green meaning deleted. Error reduction is good.
@rongpengzhang I've committed suggested IDD changes. Language about curve types may need to change slightly now from "Curve object type should be " to "Recommended curve object types are:" ?

@rongpengzhang
Copy link
Contributor Author

@Myoldmopar I agree that this should be consistent with other similar cases in EnergyPlus. If the tradition is to consider it a "waning", it is fine with me.

@rongpengzhang
Copy link
Contributor Author

@mjwitte Thanks for the suggestion. I will do that

@mjwitte
Copy link
Contributor

mjwitte commented Feb 19, 2015

@rongpengzhang @Myoldmopar Arrrgh. I generally forget that tables can have a secondary curve type associated with them. To me, the power of tables is to not use a specific curve form (i.e. Interpolation Type != EvaluateCurveToLimits). @mmAtTs Is our expert on validation of curve types. What are the other components doing?

@Myoldmopar
Copy link
Member

@rongpengzhang @mjwitte Updated transition source to write out the default value rather than a blank. I'm not going to retransition those idfs. They will get re-transitioned before release, plus I bet other idd changes are looming anyway. So those fields will show up then. I think @mmAtTs can confirm that the code is doing the right thing on the table, and then this should be OK.

Thanks for all the review time and conversation everyone.

@mitchute
Copy link
Collaborator

@mjwitte It's been a while since I looked at the curves, but if I remember correctly we found out that the CurveManager is doing a good job of sorting out the right tables for the right types of curves. So long as the curves are referenced as they normally are throughout (which I think they are), I think this should be fine.

@Myoldmopar
Copy link
Member

OK, CI should pick it up and hopefully overnight it will come out all clean and we can merge it in tomorrow morning.

@rongpengzhang
Copy link
Contributor Author

@Myoldmopar I have put the EngRef and IORef modifications into this folder: EnergyPlusBuildSupport/docs/src/mods/for_2015_03_release/ZRP-66516336-LBNLChillerCurve/

@Myoldmopar
Copy link
Member

Thanks @rongpengzhang for the doc changes, I'll look them over. I ran the new example file locally and found some very minor messages (1 unused construction, etc.). I'm not worried about that.

@mjwitte Does ExpandObjects write out reformulated EIR chillers? If so, this will have to have an ExpandObjects update too, I guess. Sorry I just thought about that, but

  • ExpandObjects changes?

is at the bottom of my review checklist... Moving it toward the top so it's not an afterthought.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 20, 2015

@Myoldmopar Reformulated EIR chiller is referenced, so there will be a few lines of code that need to change.

@Myoldmopar
Copy link
Member

Hooray! OK, well I want this in today, so I will take a crack at making the changes in ExpandObjects. I'll tag you for review if you can just glance over the ExpandObjects changes there.

@mjwitte
Copy link
Contributor

mjwitte commented Feb 20, 2015

@Myoldmopar Search epfilter.f90 for 'reformulated'. You just need to change some integer values to make sure they line up with the new object. Also that "25" at the end of the long function call.

@rongpengzhang
Copy link
Contributor Author

@Myoldmopar We are almost there! Quite happy to see the delivery of my first EPlus product ~ Thanks @mjwitte and @mmAtTs for all the review and support! Cheers ~

@Myoldmopar
Copy link
Member

@mjwitte OK, I updated the integers to account for the new field being inserted. The new field is @ A4, which is overall argument # 9. So all the node indices were incremented by 1, plus the last field index. Should be good to go I would guess.

With CI catching back up, if there are no more commits, we should have sufficient results to determine whether it is mergeable this afternoon. Thanks for the help.

Oh, and since a separate PR went in this morning I'll merge develop right quick.

@Myoldmopar Myoldmopar self-assigned this Feb 21, 2015
Myoldmopar added a commit that referenced this pull request Feb 21, 2015
Additional Chiller Part Load Curve Type
@Myoldmopar Myoldmopar merged commit 5882b18 into develop Feb 21, 2015
@Myoldmopar Myoldmopar deleted the LBNLChillerCurve-2 branch February 21, 2015 01:36
@Myoldmopar Myoldmopar changed the title Additional Chiller Part Load Curve Type Implement additional chiller part load curve type Mar 7, 2015
@Myoldmopar Myoldmopar added the NewFeature Includes code to add a new feature to EnergyPlus label Mar 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NewFeature Includes code to add a new feature to EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants