Implementation of On-Off Control Capabilities for the Variable Flow and Electric Low Temperature Radiant Systems

[RKStrand]

Pull request overview

• "Fixes"/Implements Implementation of On-Off Control for Variable Flow and Electric Radiant Systems #8044
• This pull request implement the on-off control enhancement for low temperature radiant systems (variable flow and electric). Previously, these two systems would require a throttling range of at least 0.5C and would vary the flow/power to the system linearly through that temperature range. Some systems in practice actually use simply an on-off control. So, the requirement to have at least a 0.5C throttling range was relaxed so that it can be anything greater than or equal to zero. When the throttling range is entered as zero, the system will operate as a true on-off device. This work includes a new subroutine to calculate the flow/power fraction that is shared between the radiant systems, a new unit test, a modified IDD, and two modified IDF files to test the new on-off capabilities.

Other notes:

• No documentation changes are needed for this enhancement (the lower limit on the throttling range was an IDD limit), but another upcoming PR will enhance the I/O documentation to clarify the throttling range parameter and will address the zero throttling range situation that is now allowed.

• As the two IDF files have changed, it is expected that the output from these two files will also be different enough to result in "big diff" notifications. This is to be anticipated. Other files may show very slight changes but these are not expected to be very significant as the changes are algorithm neutral.

• The new feature proposal was included as part of the work that was added with PR#8016 that had the information regarding the on-off control work included in this PR: https://github.com/NREL/EnergyPlus/pull/8016

NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE

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: see above for 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

[Myoldmopar]

I'd like to discuss whether those minimum throttling checks should be removed or not.

[Myoldmopar]

Makes sense, users can now have zero range to give on/off control.

[Myoldmopar]

I'm not convinced this needs to be here. If I'm reading it right, these minimum throttling range blocks should just be removed entirely.

[Myoldmopar]

There should be a space before and after the division operator.

[RKStrand]

Will fix this when we resolve the minimum throttling range issue.

[Myoldmopar]

Hooray for a base class method!

[RKStrand]

Let's just say I had a great teacher! I was definitely following examples from the previous radiant work. Glad to be doing things in a way that heads E+ in the right direction.

[Myoldmopar]

Code reuse! w00t!

[Myoldmopar]

Nothing significant found in code review, but I would like to resolve a couple comments. There is one minor formatting change to make that I wouldn't worry about, but this branch has a conflict to resolve anyway, so might as well fix that up at the same time, even if nothing else changes.

I'm a little surprised to see so many diffs. I would've thought that any file that had a throttling range >= 5 would have zero diffs. Perhaps the changes are due to the branch being out of date.

The conflict is extremely small, just accepting your changes in a header file. Once we have resolution on the error checking blocks, I'm happy to pull develop in and resolve it if needed. Then perhaps another round of CI results will be cleaner. Or perhaps I'll just understand why those diffs are there.

[RKStrand]

Thanks, @Myoldmopar for looking at this PR! I can fix the formatting issue--that's not a problem, especially given that I think you want some other changes.

I too was surprised by the number of diffs. I wasn't able to look at them yesterday, but I am hoping to look at that today. There are a couple of files that should have diffs, but this extended beyond that and I'm not sure why. Maybe there was something else that was done in develop that caused the problem, but it seemed like the diffs were all in radiant file. So, I'm not sure that another change in develop is causing this problem.

What I did see in some files over the weekend was that a small change in a temperature at some point tripped the system on when in develop it might still have been off (or vice versa). That resulted in the radiant system running in the new vs not in develop. I'm not sure that explains all of the diffs though. Admittedly, I put in the PR thinking this would go smoothly. Should have waited for the results--my apologies. I hope to be looking at this more this afternoon.

As for the checks for the minimum throttling range, I am happy to take them out. My thinking was that a negative throttling range would not make any sense and technically someone could change their IDD to allow for a negative throttling range. Of course, I trap that case in the new subroutine and would treat that as a zero throttling range. I was trying to be as "controlling" as possible with my code--I guess sort of like my recent excessive use of parenthesis. Nevertheless, I'm happy to change things to whatever you want. So, what would you like to see done?

[RKStrand]

@Myoldmopar Also, I'm not sure why this is showing as having a "conflict". It seems like I just added a line to LowTempRadiantSystem.hh. Why did GitHub get confused here? Or is there some character that got changed that I am missing?

[Myoldmopar]

That resulted in the radiant system running in the new vs not in develop.

That's very reasonable, however, in those cases, the MTR file diffs should be small or zero. The timestep-by-timestep values can certainly change, but the overall energy impact should be minimal or nonexistent. If the MTR file still shows big diffs in unexpected files after you merge develop then that is more suspicious.

technically someone could change their IDD to allow for a negative throttling range

EnergyPlus doesn't actually read the IDD at runtime anymore. It is embedded inside the EnergyPlus exe file, and making changes to the IDD will not be reflected in a simulation. Your restriction on the value being >= zero will be enforced by the input processor before your code sees it. So my suggestion is to just remove those checks.

Also, I'm not sure why this is showing as having a "conflict"

It's a little hard to grok, but I think it is because you added a function to a blank line and in develop a blank line was added to that same blank line. Or something like that. The conflict resolution is just to accept your new function declaration and delete the other side of the conflict marker.

[RKStrand]

@Myoldmopar Ok, I've merged current develop into this branch and I've eliminated the conflict. I'll get rid of the checks as requested and push that up as well. Hopefully the diffs disappear, but if they don't I'll work on hunting them down. I'll let you know when I think everything is resolved and ready for re-review.

[Myoldmopar]

8 big meter diffs are still there. I can run a test suite of those locally if that would be helpful. I wonder if you could isolate the exact changes which caused the diffs. That would be helpful in justifying them. Otherwise I would just do the old fashioned "build develop and run the files with both branches and show a bunch of plots with the differences on this PR" route.

[RKStrand]

Yeah not sure. I may have to go the old fashioned route. There was only one commit for the actual code changes. So there were no intermediate commits 🙁. Just not sure why there were so many diffs. Might also look at the IDFs and see if there is any pattern that can be discerned. One file had almost all very small diffs in everything except purchased heating and those diffs were big. Not sure what would cause that because you’d think if purchased heating was very different everything else would be off too. The only thing that might be an issue is that I sort of fixed a bug along the way. The code was actually allowing a fraction of flow that could be greater than the theoretical 100% for the variable flow system. I eliminated that but didn’t think it would cause massive diffs. I could potentially rebug the code and see if that explains things?

[Myoldmopar]

I could potentially rebug the code and see if that explains things?

I highly suggest rebugging, at least locally, and see if that clears things up with local test. You are welcome to rebug and push the commit up to let CI verify for you, too.

[Myoldmopar]

@RKStrand just a heads up, a couple minutes ago I merged #8108 which will cause table diffs to show up on Linux. No big deal, but just don't be surprised to see those. I am currently building develop and this rebugged branch and will run regressions on those files showing diffs earlier.

[RKStrand]

Thanks for the heads up, @Myoldmopar! I was just about to post something here but got temporarily distracted.

So, the commit has a description of the "bug" that I put back in the code. I say "bug" in quotes because there is potentially an argument that could be made that perhaps it isn't a bug. I'll document it here and if rebugging the code gets rid of the extra diffs in the output, then we can decide what we want to do.

In the variable flow radiant system, the code calculates a flow fraction based on the temperature difference between the controlled temperature and its current setpoint and the throttling range. It's a simple ratio: flowFrac = tempDiff/throttlingRange (see I even used C++ naming here 😀). If tempDiff is greater than throttlingRange, then this comes back as greater than 1.0. It multiplies the flowFrac by the user's maximum flow rate so in theory flowFrac shouldn't be greater than 1.0. However, the code before this work didn't flag that case--it just let things proceed with a value greater than 1.0. Perhaps the assumption was that the plant flow resolver would allow this if flow available, but that seems dubious. And I'll be honest--I simply don't remember if I did this intentionally or on purpose. Hence, I am not 100% sure whether this is officially a bug or not. I personally think it is, but then why not allow flowFrac to be greater than 1.0 if more flow is available and not being used.

In contract, the electric radiant system has always had code to avoid going over heatFrac of 1.0 since the user system capacity should be a strict limit. The questions are:

• Is this really a bug then?

• If this is determine to be a bug, should I just fix it here with this other work and be done with it or log it as a separate defect?

So, if this "bug" ends up being the cause of the other diffs (there are three files that will have big diffs because their IDFs changed), how do we proceed?

[Myoldmopar]

Running locally, comparing develop to the rebugged version, I tested these 13 files which were showing the bulk of diffs above:

RadLoTempHydrHeatCool
RadLoHydrHeatCoolAuto
RadLoTempHydrHeatCoolDryCondFD
RadLoTempHydrInterMulti
RadLoTempHydrHeatCoolDry
RadLoHydrHeatCoolAutoCondFD
RadLoTempHydrCtrlOpt2
RadLoTempHydrTermReheat
RadLoTempHydrCoolTower
RadLoTempHydrCtrlOpt
RadLoTempHydrHeatCool2D
RadLoTempHydrCoolTowerCondFD
RadLoTempHydrMulti10

RadLoTempHydrHeatCool is the only one that shows diffs. Note that I didn't run diffs on RadLoTempElecCtrlOpt2.idf or RadLoTempHydrCtrlOpt3.idf as they weren't in my regression suite csv list yet.

At this point CI is probably almost done so we'll see if it confirms. Again there will be a ton of table diffs unrelated to this change.

[RKStrand]

Thanks, @Myoldmopar! Would it be worth merging in develop so we can make a clean compare?

[Myoldmopar]

No, ESO and MTR results have not moved in develop, just table diffs. This set of CI results should show whether the MTR and ESO diffs are cleaned up by your rebugging and then we'll decide where to go next.

[RKStrand]

One more point--the two files that I would expect big diffs in the output are: RadLoTempHydrHeatCool and RadLoTempElecCtrlOpt2.idf. Everything else I would expect to be identical or very very small diffs. So, it sounds like rebugging the code "fixes" the problem, correct?

[Myoldmopar]

And boom. Ignore the table diffs, and just focus on this:

• 2 tests had: EIO diffs.
• 2 tests had: ESO big diffs.
• 2 tests had: MTR big diffs.
• 1 test had: ERR diffs.

Looks like your rebugging got things back to where you expect? The dashboard hasn't rebuilt yet, but I can see the two files with big ESO diffs are:

• RadLoTempElecCtrlOpt2
• RadLoTempHydrHeatCool

Once the dashboard rebuilds in a few minutes you can inspect the diffs there to verify.

Looks like your bug fix is indeed the culprit.

[RKStrand]

Oh sweet--that is just awesome and confirmation that the "bug fix" is the culprit. Those two files that are showing diffs should have big diffs because the IDFs changed (I changed some of the systems in those IDFs to on-off control from throttled control). With nothing else showing a diff, this is exactly what I would expect in the results.

So, we have an answer. Now, how should we proceed? We can leave as is and merge this. Or I can refix the "bug" and do another commit and then it could be merged. So, what are your thoughts on:

Do you agree that the fix is correct? If yes, do we incorporate that fix with this PR or separate it out into a new issue? I can easily refix the issue (is there a way to revert a commit or back it out?).

If you think the fix may not be appropriate, then I think we should just move forward with what we currently have.

Anyway, let me know what you think and I'll get this taken care of as soon as I can.

[Myoldmopar]

Let's leave this as-is and fix it in a follow-up issue.

[RKStrand]

@Myoldmopar 👍 So, if you are happy with where things are, then I think this is ready go in. I can add a new issue describing the problem.

[Myoldmopar]

Go for it. I'll let CI wrap up and then take a final peek, but yeah, I think it's likely ready to go.

[Myoldmopar]

Good to go!

[Myoldmopar]

Yay gone!

[Myoldmopar]

Your editor left some trailing white space on this line, no worries for now, but I'd check to see if you can tell it to remove them upon saving the file.