Issue1576 Heat pump model integration

[FWuellhorst]

I have added the heat pump model from the AixLib with all relevant submodels. During this implementation, I've added some changes (easier design, more safety control) and followed the naming convention more thoroughly.

The last remaining todo would be to add more Examples and maybe a Validation package with data from the paper.
But before I add these, a review of the underlying structure etc. would be good.

[FWuellhorst]

@mwetter : I added all models and features as described in #1576. From my point of view it's now (finally) ready for merging.
If the Travis CI fails, I will try to fix it.
As the PR is rather large, is it possible to split up some sections? Or do you want to review everything together?

[hcasperfu]

@FWuellhorst Greetings!
I will be doing the initial round of code review and work with you to ensure that the code complies with format requirements and conventions, before the review of the code functionality and implementation.

• Casper

[hcasperfu]

@FWuellhorst
Please see in-line comments. Please also refer to the style guide.

This is the first round of review where I mainly suggested changes to comply with the style and format conventions. I only looked at the codes briefly and didn't go through everything, so please bear with me if I mistakenly marked something that was intentional.
Please let me know when you are done and I will do another round of style review.

Cheers,
Casper

[FWuellhorst]

@hcasperfu Thank you very much for this first round of style check review. I included most of your comments, please check my answers in some comments. Especially regarding the naming rule, we sometimes make exceptions in the AixLib if the longer name improves understanding of the model. What is your take on that?

[FWuellhorst]

Maybe a note on the review-order regarding documentation. I did not update the existing documentation, as maybe some functions will be changed due to the review process. Would it be possible to do a functional review and then a style review with regard to documentation? Or should I update the documentation now and adjust if necessary?

[mwetter]

Maybe a note on the review-order regarding documentation. I did not update the existing documentation, as maybe some functions will be changed due to the review process. Would it be possible to do a functional review and then a style review with regard to documentation? Or should I update the documentation now and adjust if necessary?

I think it would make sense to first give a verbal feedback if we set aside half an hour or an hour for a meeting to discuss higher level feedback before going down into details.

[FWuellhorst]

@hcasperfu @mwetter :

I went through all models and checked syntax, documentation, icons, and 80 column line width. I agree that a short meeting would help. Are you free on Friday morning? Else, next week looks better for me.

[hcasperfu]

I went through all models and checked syntax, documentation, icons, and 80 column line width. I agree that a short meeting would help. Are you free on Friday morning? Else, next week looks better for me.

Sounds good! I plan to do another round of format review later this week.
If I am needed at the meeting, I am available this Friday except 09:00-09:30 (GMT-7).

[hcasperfu]

Please see inline comments.
This concludes the second round of format review. I mainly

• caught variable names that did not comply with the naming convention,
• caught variables missing description strings,
• and tried to make some long descriptions more concise.

Also as a side note, although I didn't pay too much attention to the content, I feel I saw some code repetitions. They may benefit from improved inheritance structure. But this is a discussion for the functionality review.
Good work and see you at the meeting!
Casper

[FWuellhorst]

@hcasperfu : Please check my comment regarding partial models, I can't seem to get your implementation to work without errors. Other than this minor request, I accepted everything and revisited style guides, added documentation etc.

@mwetter: I will try to fix the travis CI for the existing .mos-scripts.
I added an extensive UserGuide and hope that the current implementation fits IBPSA requirements.
I also added several new features for the models which could enable direct usage of ASHRAE based devices.

Even though there is still room for improvement (e.g. dp_nominal could be extracted from datasheets), such new features would require substantial research and modelling time. We can tackle those improvements in future issues in my opinion.

[hcasperfu]

@FWuellhorst -
Greetings! I clarified what I meant regarding the partial keyword in the inline comment.
I think it would be good if we can address the errors in the CI tests first. Let me know if you want to set up a meeting so that we can look at those together.

[FWuellhorst]

Yes, let's do that!
I am quite packed the upcoming three weeks during the evenings.
From what I see, it's mainly the fact that the example checks fail. I assume due to warnings in Dymola? And OM translation fails, I will take a look at that. Branch and PR pipelines are the same, or?

[FWuellhorst]

@hcasperfu @mwetter : From what I see, the examples fail because there are Warnings in the model checks:

Due to the non-literal value in the nominal m_flow_nominal attribute introduced in the PartialResistance model.
I calculate the mass flow rate using the cp of the Medium. Apparently, this raises this non-literal warning. I can set the cp values in the examples directly, but that would show the "wrong" usage of the model. It works in OM so I think it is a Dymola issue. However, I would not wait for a fix on their side. I will overwrite the default for cp for now to check if CI passes.

As these warnings have bugged me and other IBPSA / AixLib users for quite some time: Why is this if condition in the nominal attribute even necessary? You implemented this for #408 (39e8628) quite some time ago @mwetter, but I don't see why a mass flow rate does not allow for a zero nominal attribute? Or why setting the nominal or min of m_flow_nominal to something different from 0 does not work.

[hcasperfu]

@FWuellhorst -
I'm not sure the non-literal values were what caused the tests to fail. When I do a pedantic check with IBPSA.Fluid.Chillers.Examples.ModularReversible, Dymola finds undeclared switch variables used in various Dialog.enable attributes and produces error messages (instead of warnings). For example (fe7887b):

Error: Undeclared variable:  use_autoCalc
in the Dialog.enable attribute of modRevChi.mEva_flow_nominal declared in class IBPSA.Fluid.Chillers.ModularReversible
Variable modRevChi.mEva_flow_nominal was declared in class Modelica.Units.SI, /tmp/tmp-IBPSA-0-nzexf1ot/IBPSA/Fluid/HeatPumps/BaseClasses/PartialReversibleRefrigerantMachine.mo at line 126, and used in component modRevChi.PartialReversibleRefrigerantMachine.
Error: Undeclared variable:  use_minFlowCtrl
in the Dialog.enable attribute of modRevChi.safCtrPar.m_flowEvaMinPer declared in class IBPSA.Fluid.HeatPumps.SafetyControls.RecordsCollection.DefaultHeatPumpSafetyControl
Variable modRevChi.safCtrPar.m_flowEvaMinPer was declared in class IBPSA.Fluid.HeatPumps.SafetyControls.RecordsCollection.PartialRefrigerantMachineSafetyControlBaseDataDefinition, /tmp/tmp-IBPSA-0-nzexf1ot/IBPSA/Fluid/HeatPumps/SafetyControls/RecordsCollection/PartialRefrigerantMachineSafetyControlBaseDataDefinition.mo at line 72, and used in component modRevChi.safCtrPar.PartialRefrigerantMachineSafetyControlBaseDataDefinition.
Error: Undeclared variable:  use_autoCalc
in the Dialog.enable attribute of modRevChi.mCon_flow_nominal declared in class IBPSA.Fluid.Chillers.ModularReversible
Variable modRevChi.mCon_flow_nominal was declared in class Modelica.Units.SI, /tmp/tmp-IBPSA-0-nzexf1ot/IBPSA/Fluid/HeatPumps/BaseClasses/PartialReversibleRefrigerantMachine.mo at line 65, and used in component modRevChi.PartialReversibleRefrigerantMachine.

The same errors messages were generated in the fail log file when I ran the CI tests locally.
I would suggest looking into these first.

[FWuellhorst]

Thanks for the hint! I fixed all errors appearing with pedantic mode on.

Is there a tutorial on how to run the CI locally? I have a dymola docker, which python scripts do I need to execute?

[hcasperfu]

@FWuellhorst -
The CI tests are the reason I suggested we set up a meeting.
I will give you some quick tips over email. Those are too long (and tedious) for the PR post.

[hcasperfu]

@FWuellhorst -
All CI tests pass now! I will look at this PR in more details, likely next week.

[hcasperfu]

This is the last round of format review, mainly focused on clarity and crispness of documentation. I suggested various language edits through the inline comments. Most of them are straightforward suggestions and can be directly committed.

• They are mostly suggestions, because I want to make sure I understood them correctly. For clear typos, I made pushes instead.
• A lot of them shorten long three-line sentences after a class or variable declaration - those details should be reserved for the documentation.
• There are various places where a reference or source link (e.g. "the UK standard", "the German standard", "the conference paper", etc.) should be provided.
• There are a few expressions commonly used throughout this package that I find confusing or misleading (both as variable names and as in the description/documentation).
  • Related to the safety control blocks:
    • "Runtime" and "lock time"/"loctime" - I suggest calling them "on-time" and "off-time" (the latter is already used in the documentation of Controls.Safety.OnOff).
    • "Runs per hour" can be called "cycle ratio".
  • "Useful side" and "not useful side" of the chiller/heat pump look like very awkward expressions to me. I suggest perhaps calling them the "interior side" and "exterior side".
  • Whenever the word "power" is used in the context of a chiller or heat pump, please be clear if it is the "electric power consumption", or "heating/cooling output". This comes from teaching experiences where students seem to confuse these often, because they all can use the same unit.

There is one issue that needs to be fixed before this PR can move forward.

• Example/validation models under Fluid.HeatPumps.ModularReversible.Controls.Safety.Examples don't have .mos scripts. Please add those and ping me to create the results files for you.

Other items up for discussion.

• As we chatted in our meeting, there are extensive (in my opinion) code repetitions between some of the sister class pairs, namely LargeScaleWaterToWater, ModularReversible, ReversibleCarnotWithLosses at the top level and RefrigerantCycle.EuropeanNorm2D. Trying to combine them may be worthwhile in terms of code maintainability, if this doesn't increase the inheritance complexity too much.
• A small new package for string signal blocks IBPSA.Utilities.IO.Strings was created in this PR. It looks suitable for general use and is lightweight (unlike EvaporatorCondenserWithCapacity which we discussed in our meeting). I believe it is fine to have it outside of the ModularReversible package, but the documentation needs to be revised to remove any language that makes it seem to have specific use only. Basically, either move this package to ModularReversible, or delete all "Used to enable String comparison in optional models".

This concludes the last round of format review from me.

[FWuellhorst]

@hcasperfu: First, thanks for the really nice review and all the comments. I responded to some of them for clarifications or to state my opinion, please check those first.

Regarding your longer message:

• I will add the sources to all data-sheets and other references together with adding the pressure curve regressions as I have to open the data sheets anyway.
• I added the .mos scripts, can you generate the results for me?

"Runs per hour" can be called "cycle ratio".

I disagree, it is the number of starts per hour. The term ratio indicates some ratio, but it is an absolute value.

"Useful side" and "not useful side" of the chiller/heat pump look like very awkward expressions to me. I suggest perhaps calling them the "interior side" and "exterior side".

Interior and exterior sides do not make sense here. Please check my comment in your suggested change.

As we chatted in our meeting, there are extensive (in my opinion) code repetitions between some of the sister class pairs, namely LargeScaleWaterToWater, ModularReversible, ReversibleCarnotWithLosses at the top level and RefrigerantCycle.EuropeanNorm2D. Trying to combine them may be worthwhile in terms of code maintainability, if this doesn't increase the inheritance complexity too much.

As discussed, I still think this adds a lot of complexity for new users. I am against adding extra partial classes.

A small new package for string signal blocks IBPSA.Utilities.IO.Strings was created in this PR. It looks suitable for general use and is lightweight (unlike EvaporatorCondenserWithCapacity which we discussed in our meeting). I believe it is fine to have it outside of the ModularReversible package, but the documentation needs to be revised to remove any language that makes it seem to have specific use only. Basically, either move this package to ModularReversible, or delete all "Used to enable String comparison in optional models".

I made the doc more generic.

[hcasperfu]

@FWuellhorst -
Thanks for the revision.

• Concerning the unit tests, I have generated the new result files.
  • There is one other model currently failing: IBPSA.Fluid.HeatPumps.ModularReversible.Examples.ReversibleCarnotWithLosses_OneRoomRadiator.
• The "runs per hour" can be called "cycle rate" (e.g. used on this website). I apologise that I misused the "ratio" term which refers to something else.

[FWuellhorst]

@hcasperfu : Thanks for the reference results. If fixed the failing example, this may lead to a new reference result as I changed a structural parameter.

The cycle rate is a good name, thanks!

[FWuellhorst]

Additional review comments from @mwetter :

• Use table data in K and don't convert to degC
• Add COP outputs
• Shorter instance names in examples
• Minor naming comments

[FWuellhorst]

@mwetter: Regarding COP outputs:
I added the outputs on the modular level. This means the cooling model adds an EER, the heating model an COP.
However, I will not go ahead and add an actual output connector on the heat pump level. This would require multiple further outputs, such as temperatures, power consumption, etc., which are as important as the efficiency. We use the bus connector to aggregate all these important values, but we disabled this as required by IBPSA.

Regarding COP with inertia: I don't see the added value, as this will lead to further weird values if inertia are considered and no difference to the ones from the already added efficiencies if inertia is not modelled.

[mwetter]

@FWuellhorst : b4d34a2 conditionally enables sigBus (enabled for AixLib, but not for the other libraries). A caveat is that if you save the model in Dymola, then Dymola may reformat the file and move the two comment lines together, at which point the enabling/disabling does not longer work.

Can you please finish up the implementation, as various models fail the regression test due to unknown data records.

[mwetter]

@FWuellhorst :

• Can you please add to the UsersGuide how scaFac is used? I don't think this is explained anywhere, and I found it not clear. As a user, I would expect scaFac to be a top-level parameter that if set to say 1.2, then the HP is 20% larger, allowing an easy test of different sizes while still using the performance curves from the data sheets or tables.
  Also, there is scaFacHea and scaFacCoo, which may be different up to some tolerance. Wouldn't they be the same if the heat pump were scaled by 20%?

[FWuellhorst]

@mwetter This behavior was changed as requested by @AntoineGautier. The scaFac now only exists for the table-data models, the Carnot models do not use them.
In general, if you want different sizes, just change QHea/Coo_flow_nominal. These values are used to scale the models internally. For tables, scaling factors are used, for Carnot, no scaling is required. This approach is the same as the current Carnot_ models.

From the UserGuide:

  Using the nominal conditions and the specified heat flow rate, 
  the nominal electrical power consumption <code>PEle_nominal</code> is calculated. 
  As reversible devices have typically a four-way-valve and a single
  compressor, you have to make sure that the values for <code>PEle_nominal</code> 
  are similar between heating and cooling. The pre-configured models 
  warn about deviations if they are too large.

The descriptions holds for all models types, as the scaling factor in turn is used to calculate the nominal electrical power. For table-data however, it's best to warn about different scaling, as the nominal electrical powers may differ in reality, as well.
The documentation of IBPSA.Fluid.HeatPumps.ModularReversible.TableData2D and IBPSA.Fluid.HeatPumps.ModularReversible.RefrigerantCycle.TableData2D further explains the scaling factor.

Does this clarify your concern?

[mwetter]

@FWuellhorst : Please see the inline comment for a couple of places where I need your help. Thanks.

[mwetter]

@FWuellhorst Please see inline comment for some more feedback.

[mwetter]

@FWuellhorst : Thanks again for all your hard work on implementing these large models! They are ready to be merged.