Replace PlugFlowPipe outlet from vectorized to single port #1503

bravache · 2021-07-12T21:12:57Z

This PR addresses the problem of compatibility of IBPSA.Fluid.FixedResistances.PlugFlowPipe with other pipe classes, by changing its outlet port from a vectorized port to a single port.

The PR brings non-backward compatible changes, for which a conversion script is provided for Dymola users.

Validation results

The following example and validation models uses the PlugFlowPipe class and were updated to reflect that change:

IBPSA.Fluid.FixedResistances.Examples.PlugFlowPipe
IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.FlowReversal
IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.PlugFlowAIT
IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.PlugFlowULg
IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.TransportWaterAir

For most of the models, the only change required was to remove the parameter nPorts from the parameterization of PlugFlowPipe and update the outlet connection. Additionally, for PlugFlowAIT, two splitters had to be introduced to replace the connections out of pip1 and pip5.
The results of all example/validation models remained unchanged, with the exception of PlugFlowAIT which only shows an error in the initial step of the simulation, but no error for the remainder of the runtime.

On a side note, the pressure boundary condition for the FlowReversal example was updated to 5bar, to avoid negative pressure at the inlet when the flow is reversed. This does not change the expected results, but make the example more robust.

Backward compatibility

This change is non-backward compatible. A conversion script was written to redirect previous objects to the obsolete class.

bravache · 2021-07-13T21:15:24Z

@AntoineGautier Please take a look and let me know if I've missed something. Thanks!

AntoineGautier

@bravache Please see my inline comments.
Also, and as proposed in the issue page, I would merge PlugFlowCore and PlugFlowPipe into one, making the fluid volume conditional to an advanced Boolean parameter which would be true by default. The two models are too similar in my opinion to justify keeping them both.

AntoineGautier · 2021-07-15T12:26:32Z

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo

 <br/>
 This mixing volume is not present in the
 <a href=\"modelica://IBPSA.Fluid.FixedResistances.BaseClasses.PlugFlowCore\">PlugFlowCore</a> model,


I would delete that sentence as we agreed that the mixing volume was necessary to represent the effect of the pipe wall capacitance (not only for decoupling the DAE between multiple instances).
Instead I would add something like:

Note that in order to model a branched network it is recommended to use IBPSA.Fluid.FixedResistances.Junction at each junction and to configure that component with a state (see for instance
<a href="modelica://IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.PlugFlowAIT">
IBPSA.Fluid.FixedResistances.Validation.PlugFlowPipes.PlugFlowAIT).
This will avoid the numerical Jacobian that is otherwise created when the inlet ports of two instances of the plug flow model are connected together.

I've reworded the whole documentation to incorporate part of the PlugFlowCore doc. Let me know if that works.

AntoineGautier · 2021-07-15T12:36:36Z

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo

@@ -1,7 +1,7 @@
 within IBPSA.Fluid.FixedResistances;
 model PlugFlowPipe
  "Pipe model using spatialDistribution for temperature delay"
-  extends IBPSA.Fluid.Interfaces.PartialTwoPortVector;


IBPSA.Fluid.Interfaces.PartialTwoPortVector may be retired into Obsolete as it was only used by the plug flow model.
To be validated with the IBPSA team.

I might keep it like that since it predates the PlugFlowPipe model. Also, in the same package, there is also the PartialFourPortParallel which is not in used.

I filed a ticket so that it can be discussed before implemented here: #1504

bravache · 2021-07-19T15:52:58Z

I've moved the PlugFlowCore to Obsolete, and expanded its content directly in PlugFlowPipe. I've made the mixing volume conditional on the value of have_PipCap, and was also able to reuse the former PlugFlowCore validation results to verify the results of the model when that Boolean is false.

AntoineGautier

I just have one minor change request and a suggestion.
Otherwise the changes look good to me.

IBPSA/Fluid/FixedResistances/Examples/PlugFlowPipeNoMix.mo

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo

bravache · 2021-08-18T17:45:59Z

@Mathadon @nytschgeusen @cvering @mwetter
This PR is ready to merge. Antoine had a couple of comments left, but those were addressed and he is now on break, so I think it is reasonable to move forward.

Let me know if you have any comments. Otherwise, this will be merged on 09/03/2021.

Content

The main change introduce is changing the vectorized outlet port of PlugFlowPipe to be a single outlet port, which is consistent with other pipe model in the same package.

Additionally, the Fluid.FixedResistances.BaseClasses.PlugFlowCore and Fluid.FixedResistances.PlugFlowPipe are combined into a single model (previously the latter was a wrapper around the former), and the outlet mixing volume is made conditional on a boolean parameter have_pipCap.

The previous classes were moved to Obsolete and a conversion script was written to update PlugFlowCore and PlugFlowPipe instances to these obsolete classes.

The existing unit tests stored output had to be updated to reflect the change in translation statistics, but the timeseries results remained unchanged.

Mathadon · 2021-08-25T08:38:31Z

@bravache thanks for the summary! I have forwarded this to a colleague who uses this model more actively than I do.

mwetter · 2021-09-13T15:43:44Z

Michael to merge after last review.

Otherwise, the rename script will not update them if the classes are moved to another package

See https://github.com/ibpsa/modelica-ibpsa/pull/1503/checks?check_run_id=3593090402

…a/modelica-ibpsa into issue1494_plugflowpipe_singleoutlet

See https://github.com/ibpsa/modelica-ibpsa/pull/1503/checks?check_run_id=3593443631

This is used by Buildings/Fluid/FixedResistances/PlugFlowDiscretized.mo

Updated reference results, only minor changes

mwetter · 2021-09-14T15:32:44Z

Todo

Consider removing constant computeFlowResistance in the new pipe model. This would require a refactoring in the Buildings library for the distributed pipe model, which would in turn allow eliminating states.

mwetter · 2021-09-15T18:56:29Z

@Mathadon : This is ready to merge from my point of view, but I think it would be good to have another approvals as during the clean up, I changed in PlugFlowPipe most instances to be protected, and instead exposed and/or introduced new variables QEnv_flow (for heat exchange with environment), dp, m_flow and v.
QEnv_flow = heatPort.Q_flow was introduced because in Buildings (and maybe soon in IBPSA) there is a discretized version of this model, in which QEnv_flow = sum(heatPort[:].Q_flow). By introducing this variable, both models have the same high level variables.

Mathadon · 2021-09-16T11:40:55Z

@mwetter I'll try to review this tomorrow!

Mathadon

My largest concern is that the model IBPSA.Fluid.FixedResistances.PlugFlowPipe is not yet symmetric: a mixing volume consists at just one of the two pipe outlets/inlets. I propose to use two mixing volumes, each with half the mass.

Mathadon · 2021-09-17T10:19:01Z

IBPSA/Fluid/FixedResistances/BaseClasses/PlugFlowTransportDelay.mo

@@ -48,8 +49,7 @@ initial equation
  t0 = time;

 equation
-  u = m_flow/(rho*(dh^2)/4*Modelica.Constants.pi)/length;
-
+  u = m_flow * conUM;


Mathadon · 2021-09-17T10:20:08Z

IBPSA/Fluid/FixedResistances/Examples/PlugFlowPipe.mo

+    annotation (Placement(transformation(extent={{-30,10},{-10,30}})));
+  Sensors.TemperatureTwoPort senTemInNoMix(
+    redeclare package Medium = Medium,
+    m_flow_nominal=1,


this could use tau=0?

Added for all temperature sensors.

Mathadon · 2021-09-17T10:20:41Z

IBPSA/Fluid/FixedResistances/Examples/PlugFlowPipe.mo

+    annotation (Placement(transformation(extent={{0,-30},{20,-10}})));
+  Sensors.TemperatureTwoPort senTemOutNoMix(
+    redeclare package Medium = Medium,
+    m_flow_nominal=1,


Same for all other examples to be honest: the filter slows down the model and skews the results.

Mathadon · 2021-09-17T10:23:10Z

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo

+    "Control volume connected to port_b. Represents equivalent pipe wall thermal capacity."
+    annotation (Placement(transformation(extent={{70,20},{90,40}})));
+
+  LosslessPipe noMixPip(


why not use a simple connection instead?

I believe I've written it like that to conserve a symmetrical syntax between the case with the mixing volume (have_pipCap) and the case without a mixing volume (not have_pipCap).
Also, in the graphical editor of Dymola, you get an annotation in the hover tooltip when a component is optional (screenshot below), but not when a connect statement is optional, which makes that structure a bit more explicit.

That being said, if there is a computational or syntax issue with that format, I can switch back to an optional connect.

Mathadon · 2021-09-17T10:27:13Z

IBPSA/Fluid/FixedResistances/Validation/PlugFlowPipes/PlugFlowAIT.mo

+    redeclare package Medium = Medium,
+    m_flow_nominal={pip1.m_flow_nominal,pip5.m_flow_nominal,pip4.m_flow_nominal},
+    from_dp=true,
+    linearized=true,


this has no purpose if dp_nominal=0?

Indeed! Good catch. Removed for both junctions in this model.

Mathadon · 2021-09-17T10:28:48Z

IBPSA/Obsolete/Fluid/FixedResistances/BaseClasses/Validation.mo

+package Validation "Collection of validation models"
+  extends Modelica.Icons.ExamplesPackage;
+
+  model PlugFlowCore "Simple example of plug flow pipe core"


It would be better to split this package in multiple .mo files?

Good catch! Updated

bravache

Thanks Filip, I've reviewed your inline comments.
With regard with the symmetry of the model, I was reluctant to introduce it because when I implement it naively, models that have two PlugFlowPipe in series become structurally singular because of the two connected mixing volumes.
I am discussing this with Michael and will update you if we come up to a solution.

Edit: Actually you can disregard that message, the structural singularity was a different issue from my rushed implementation.

bravache · 2021-09-23T21:03:20Z

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo

+    "Control volume connected to port_b. Represents equivalent pipe wall thermal capacity."
+    annotation (Placement(transformation(extent={{70,20},{90,40}})));
+
+  LosslessPipe noMixPip(


I believe I've written it like that to conserve a symmetrical syntax between the case with the mixing volume (have_pipCap) and the case without a mixing volume (not have_pipCap).
Also, in the graphical editor of Dymola, you get an annotation in the hover tooltip when a component is optional (screenshot below), but not when a connect statement is optional, which makes that structure a bit more explicit.

That being said, if there is a computational or syntax issue with that format, I can switch back to an optional connect.

bravache · 2021-09-23T21:04:26Z

IBPSA/Fluid/FixedResistances/Examples/PlugFlowPipe.mo

+    annotation (Placement(transformation(extent={{-30,10},{-10,30}})));
+  Sensors.TemperatureTwoPort senTemInNoMix(
+    redeclare package Medium = Medium,
+    m_flow_nominal=1,


Added for all temperature sensors.

bravache · 2021-09-23T21:08:44Z

IBPSA/Fluid/FixedResistances/Validation/PlugFlowPipes/PlugFlowAIT.mo

+    redeclare package Medium = Medium,
+    m_flow_nominal={pip1.m_flow_nominal,pip5.m_flow_nominal,pip4.m_flow_nominal},
+    from_dp=true,
+    linearized=true,


Indeed! Good catch. Removed for both junctions in this model.

bravache · 2021-09-23T21:10:53Z

IBPSA/Obsolete/Fluid/FixedResistances/BaseClasses/Validation.mo

+package Validation "Collection of validation models"
+  extends Modelica.Icons.ExamplesPackage;
+
+  model PlugFlowCore "Simple example of plug flow pipe core"


Good catch! Updated

mwetter · 2021-09-23T22:12:17Z

@bravache : Above, there is the declaration:

Sensors.TemperatureTwoPort senTemInNoMix(
    redeclare package Medium = Medium,
    m_flow_nominal=1,

This looks wrong. All models use m_flow_nominal = 1 kg/s, but the source uses 3 kg/s. This should be made consistent.

bravache · 2021-10-05T16:54:45Z

@mwetter
Following our conversation and the test that showed that the computational impact was low in a sufficiently detailed model, I've implemented the symmetry in the PlugFlowPipe class. I've also added an advanced boolean flag have_symmetry which allows disabling the inlet mixing volume.
In addition, I've made the model extends from PartialTwoPortInterface rather than PartialTwoPort, since the former is more constraining, and adds show_T as a parameter.

Using single line entries avoids very long strings (they will be cut with '...', see FlowReversal model)

Providing the full class name allows refactoring scripts to update them

bravache added 5 commits July 12, 2021 13:55

first commit

b7803cd

fix package order

04f29e0

add documentation to obsolete package

d0bff32

fix typo

403a976

fix variable references

7c513c7

bravache requested a review from AntoineGautier July 13, 2021 21:14

bravache mentioned this pull request Jul 13, 2021

DHC Buried Pipes lbl-srg/modelica-buildings#2409

Merged

Minor changes [ci skip]

c9d7e1d

AntoineGautier reviewed Jul 15, 2021

View reviewed changes

AntoineGautier added the non-backward compatible label Jul 15, 2021

Expand PlugFlowCore in PlugFlowPipe

9eccb21

fix syntax

333dd4f

bravache mentioned this pull request Jul 19, 2021

Deprecate unused classes in IBSPA.Fluid.Interfaces #1504

Open

bravache and others added 3 commits July 19, 2021 14:55

fix test variable name

7304835

Update doc

655e53c

Rename parameter

43529e3

AntoineGautier requested changes Jul 27, 2021

View reviewed changes

IBPSA/Fluid/FixedResistances/Examples/PlugFlowPipeNoMix.mo Outdated Show resolved Hide resolved

IBPSA/Fluid/FixedResistances/PlugFlowPipe.mo Outdated Show resolved Hide resolved

bravache and others added 4 commits July 27, 2021 16:13

Merge examples

b0fbc02

Merge branch 'master' into issue1494_plugflowpipe_singleoutlet

245cc2c

Merge branch 'master' into issue1494_plugflowpipe_singleoutlet

2909c70

Merge branch 'master' into issue1494_plugflowpipe_singleoutlet

6dce628

mwetter added 4 commits September 13, 2021 15:50

Replaced links with full class name

d40c322

Otherwise, the rename script will not update them if the classes are moved to another package

Introduced parameter for complex parameter expression

8bbc228

Updated documentation

784ce99

Fixed drawing into pane

670f3ce

mwetter added 7 commits September 13, 2021 17:26

Removed setup tools

a637e26

See https://github.com/ibpsa/modelica-ibpsa/pull/1503/checks?check_run_id=3593090402

Merge branch 'issue1494_plugflowpipe_singleoutlet' of github.com:ibps…

5031c9b

…a/modelica-ibpsa into issue1494_plugflowpipe_singleoutlet

Moved conversion to top of file (as has been done with other entries)

7d31f46

Removed setup tools

73e7d11

See https://github.com/ibpsa/modelica-ibpsa/pull/1503/checks?check_run_id=3593443631

Added constant to suppress calculation of flow resistance

bb0338c

This is used by Buildings/Fluid/FixedResistances/PlugFlowDiscretized.mo

Updated variable names in CI tests

072086f

Corrected error in newly introduced constant.

1fc20a4

Updated reference results, only minor changes

mwetter added 2 commits September 14, 2021 10:26

Updated reference result for new output variables

beef79c

Removed parameter computeFlowResistance, introduced QEnv_flow

c8c7ca7

Mathadon requested changes Sep 17, 2021

View reviewed changes

mwetter mentioned this pull request Sep 21, 2021

Correct typo in comments #1526

Merged

simplify unecessary parameters

b204dc5

bravache commented Sep 23, 2021

View reviewed changes

Merge branch 'master' into issue1494_plugflowpipe_singleoutlet

146ce65

bravache added 3 commits September 23, 2021 15:28

make m_flow consistent in example

3705e42

make model symmetrical

505bb91

update documentation

0f0dbb5

bravache and others added 6 commits October 5, 2021 14:25

update documentation 2

5f64335

Changed m_flow to m_flow_nominal

6c0b7f9

Changed annotation

bd7c690

Using single line entries avoids very long strings (they will be cut with '...', see FlowReversal model)

Updated revision notes

abc385c

Providing the full class name allows refactoring scripts to update them

Removed trailing white space

9d1abcf

Merge branch 'master' into issue1494_plugflowpipe_singleoutlet

fb7d4ce

mwetter merged commit 6505919 into master Oct 12, 2021

mwetter deleted the issue1494_plugflowpipe_singleoutlet branch October 12, 2021 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace PlugFlowPipe outlet from vectorized to single port #1503

Replace PlugFlowPipe outlet from vectorized to single port #1503

bravache commented Jul 12, 2021 •

edited by AntoineGautier

Loading

bravache commented Jul 13, 2021

AntoineGautier left a comment •

edited

Loading

AntoineGautier Jul 15, 2021

bravache Jul 19, 2021

AntoineGautier Jul 15, 2021 •

edited

Loading

bravache Jul 19, 2021

bravache Jul 19, 2021

bravache commented Jul 19, 2021

AntoineGautier left a comment

bravache commented Aug 18, 2021

Mathadon commented Aug 25, 2021

mwetter commented Sep 13, 2021

mwetter commented Sep 14, 2021 •

edited

Loading

mwetter commented Sep 15, 2021

Mathadon commented Sep 16, 2021

Mathadon left a comment

Mathadon Sep 17, 2021

Mathadon Sep 17, 2021

bravache Sep 23, 2021

Mathadon Sep 17, 2021

Mathadon Sep 17, 2021

Mathadon Sep 17, 2021

bravache Sep 23, 2021

Mathadon Sep 17, 2021

bravache Sep 23, 2021

Mathadon Sep 17, 2021

bravache Sep 23, 2021

bravache left a comment •

edited

Loading

bravache Sep 23, 2021

bravache Sep 23, 2021

bravache Sep 23, 2021

bravache Sep 23, 2021

mwetter commented Sep 23, 2021

bravache commented Oct 5, 2021

Replace PlugFlowPipe outlet from vectorized to single port #1503

Replace PlugFlowPipe outlet from vectorized to single port #1503

Conversation

bravache commented Jul 12, 2021 • edited by AntoineGautier Loading

Validation results

Backward compatibility

bravache commented Jul 13, 2021

AntoineGautier left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AntoineGautier Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bravache commented Jul 19, 2021

AntoineGautier left a comment

Choose a reason for hiding this comment

bravache commented Aug 18, 2021

Content

Mathadon commented Aug 25, 2021

mwetter commented Sep 13, 2021

mwetter commented Sep 14, 2021 • edited Loading

mwetter commented Sep 15, 2021

Mathadon commented Sep 16, 2021

Mathadon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bravache left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwetter commented Sep 23, 2021

bravache commented Oct 5, 2021

bravache commented Jul 12, 2021 •

edited by AntoineGautier

Loading

AntoineGautier left a comment •

edited

Loading

AntoineGautier Jul 15, 2021 •

edited

Loading

mwetter commented Sep 14, 2021 •

edited

Loading

bravache left a comment •

edited

Loading