modelica-buildings icon indicating copy to clipboard operation
modelica-buildings copied to clipboard

Issue2912 district cooling

Open JayHuLBL opened this issue 2 years ago • 2 comments

JayHuLBL avatar Aug 22 '22 19:08 JayHuLBL

@khinkelman Please see the inline comments. I have also committed some changes aada46d, 9bd530d, 4a87098, 551c8a3, to clean up models and reference results. The model Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed to run with Optimica and OpenModelica with following errors:

See 'simulator-optimica.log' for details.
409*** Error: Simulation of Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed: 'FMUException: Failed to update the events at time: 1.572482E+07.'. Directory is '/tmp/tmp-Buildings-0-xlzir_zy'.
410Execution time = 1452.984 s
411make: *** [Makefile:150: test-optimica] Error 1
See 'simulator-openmodelica.log' for details.
385*** Error: Simulation of Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed: 'CalledProcessError: Command '['omc', 'Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS_simulate.mos']' returned non-zero exit status 1.'. Directory is '/tmp/tmp-Buildings-0-exrbuqkj'.
386Execution time = 1724.656 s
387make: *** [Makefile:145: test-openmodelica] Error 1

JayHuLBL avatar Aug 31 '22 18:08 JayHuLBL

@khinkelman Please see the inline comments. I have also committed some changes aada46d, 9bd530d, 4a87098, 551c8a3, to clean up models and reference results. The model Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed to run with Optimica and OpenModelica with following errors:

See 'simulator-optimica.log' for details.
409*** Error: Simulation of Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed: 'FMUException: Failed to update the events at time: 1.572482E+07.'. Directory is '/tmp/tmp-Buildings-0-xlzir_zy'.
410Execution time = 1452.984 s
411make: *** [Makefile:150: test-optimica] Error 1
See 'simulator-openmodelica.log' for details.
385*** Error: Simulation of Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS failed: 'CalledProcessError: Command '['omc', 'Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS_simulate.mos']' returned non-zero exit status 1.'. Directory is '/tmp/tmp-Buildings-0-exrbuqkj'.
386Execution time = 1724.656 s
387make: *** [Makefile:145: test-openmodelica] Error 1

Many thanks @JayHuLBL! I'll start working on these. For the errors above, are you able to access the 'simulator-*.log' files to see the reported error details?

khinkelman avatar Sep 01 '22 14:09 khinkelman

Hi @JayHuLBL, I hope you are doing well. Just a friendly follow up on this PR and an update from my end. As of last Friday, I can now successfully run OCT on my workstation.

Do you need anything from me to merge and close this contribution? Many thanks.

khinkelman avatar Oct 17 '22 21:10 khinkelman

@khinkelman I rerun the unit test for the models in package Buildings.Experimental.DHC.Plants.Cooling.Examples and added the Buildings.Experimental.DHC.Examples.Cooling.DirectUncontrolledETS to the exclusion list to not run the model with OpenModelica. I have no further comments.

When the CI tests passed, it should be ready for Michael to review.

JayHuLBL avatar Oct 18 '22 19:10 JayHuLBL

I will further process this after https://github.com/lbl-srg/modelica-buildings/pull/3157 is merged to the master (as https://github.com/lbl-srg/modelica-buildings/pull/3157 changes some of the underlying models).

mwetter avatar Nov 19 '22 15:11 mwetter

@khinkelman : This is ready to merge when the reference results are updated (which I can do) and when the review comment https://github.com/lbl-srg/modelica-buildings/pull/3093#pullrequestreview-1248749396 is addressed. This goes back to a revision by @ChengnanShi-Work: https://github.com/lbl-srg/modelica-buildings/blame/041de1b9ee39608da70bcaf3205c4ab24c77b7af/Buildings/Experimental/DHC/Loads/Cooling/BuildingTimeSeriesWithETS.mo#L8 Do you or @ChengnanShi-Work know what the rationale is? This should either be removed or explained if it is indeed needed.

mwetter avatar Jan 14 '23 10:01 mwetter

@khinkelman : This is ready to merge when the reference results are updated (which I can do) and when the review comment #3093 (review) is addressed. This goes back to a revision by @ChengnanShi-Work: https://github.com/lbl-srg/modelica-buildings/blame/041de1b9ee39608da70bcaf3205c4ab24c77b7af/Buildings/Experimental/DHC/Loads/Cooling/BuildingTimeSeriesWithETS.mo#L8 Do you or @ChengnanShi-Work know what the rationale is? This should either be removed or explained if it is indeed needed.

Thanks @mwetter for your review. I cannot recall the rationale for scaling facMulCoo in that way. @AntoineGautier, could you please comment here? This setting of facMulCoo=40*QCoo_flow_nominal/(-1.5E5)) came from your Combined.BuildingTimeSeriesWithETS originally, from what I can tell.

khinkelman avatar Jan 16 '23 19:01 khinkelman

These hard-coded numerical values should probably be removed. They come from an example model which was rapidly promoted as a generic class with lack of scrutiny regarding this parameter binding. The current PR #3165 provides a default assignment for facMulCoo that should rather be used. @dhblum is working on improving the documentation and the code itself which still includes some literal assignments. The rationale is the following.

  • A default effectiveness is assigned based on fan coil manufacturer data: this gives the shape of the so-called emission characteristic of the terminal unit Q_flow = f(m_flow).
  • The number of terminal units is computed based on the peak load and datasheet capacity.
  • An adjustment is made to guard against modifications of air flow or entering temperatures by the user that would be inconsistent with the number of terminal units, i.e., yielding an effectiveness above 1: this is done by adjusting facMul(Hea|Coo) at fixed effectiveness. The effectiveness can be modified by manually overwriting the default assignment of facMul(Hea|Coo).
  • What is worth mentioning is that the parameterization always ensures that the specified water dT is achieved at design flows and entering temperatures.

AntoineGautier avatar Jan 17 '23 09:01 AntoineGautier

@dhblum Is it best to clean up and merge https://github.com/lbl-srg/modelica-buildings/pull/3165 and then process this PR? For this PR, I tried to remove the assignment to facMulHea and facMulCoo, but then Buildings/Experimental/DHC/Loads/Cooling/Examples/BuildingTimeSeriesWithETS won't initialize (as the cooling coil design parameters seem to be not meaningful).

mwetter avatar Jan 17 '23 10:01 mwetter

Yes, https://github.com/lbl-srg/modelica-buildings/pull/3165 should be merged first. In that PR, the factors in question in https://github.com/lbl-srg/modelica-buildings/pull/3093#issuecomment-1384480665 are effectively moved to defaults within a base model (Buildings.Experimental.DHC.Loads.BaseClasses.Examples.BaseClasses.BuildingTimeSeries) so they won't have to be re-specified in this district cooling model.

dhblum avatar Jan 17 '23 18:01 dhblum

If all tests pass, this is ready to be merged.

mwetter avatar Jan 26 '23 23:01 mwetter

@dhblum : This is ready to merge if the unit tests succeed. Let me know if I should go ahead or whether you want to do another review.

mwetter avatar Jan 27 '23 20:01 mwetter

@dhblum : Thanks for the feedback, I addressed them in the latest commit and will merge when the tests passed.

mwetter avatar Jan 27 '23 21:01 mwetter

Sounds good, thanks @mwetter!

dhblum avatar Jan 27 '23 21:01 dhblum