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

issues with DoorDiscretised #937

Closed
Mathadon opened this issue Jun 5, 2018 · 8 comments
Closed

issues with DoorDiscretised #937

Mathadon opened this issue Jun 5, 2018 · 8 comments
Assignees

Comments

@Mathadon
Copy link
Member

Mathadon commented Jun 5, 2018

I made a simplified version of IBPSA.Airflow.Multizone.DoorDiscretizedOpen in IDEAS since DoorDiscretizedOpen is a bit too detailed in my opinion. @damienpicard suggested to compare my implementation with DoorDiscretizedOpen. From my understanding it's basically a combination of the stack effect and Bernoulli, combined with a discharge coefficient, which is also what I used in IDEAS, but without the fluid ports and without the discretisation.

I wrote down a number of questions/remarks regarding the implementation while I was figuring out what the model does exactly.

  • For a temperature difference of 0K, the mass flow rate is non-zero due to the regularisation that is used:
dVAB_flow[i] = dV_flow[i]*
      IDEAS.Utilities.Math.Functions.smoothHeaviside(x=dV_flow[i], delta=
      VZer_flow/nCom) + VZer_flow/nCom;

opendoor
This is a violation of the second law, which is frowned upon!
I'm not sure why this regularisation is required in the first place? The model somewhere mentions that zero flow should be avoided, but I don't see why. Possibly this could simply be removed.

  • The same non-zero flow implementation implies that the flow in a port never reverses. This is not physical, since the flow direction at the top of a door depends on the sign of the temperature difference. This may lead a user to take wrong conclusions.
  • I don't see why this model would require the implemented error control? I think this can be removed.
  • The model uses a discretisation nCom=10 by default, but for a simple test the difference in flow rates with nCom=2 is only 4%. We could reconsider the default value.
  • It is not that clear what the parameters hA and hB mean. I presume that these are the heights corresponding to the pressures of respectively port_a1/port_b2 and port_b1/port_a2. We could reformulate this to make it a bit clearer.
  • Html is incorrect in some of the base classes, e.g.:
<li><i>February 8, 2005</i> by Michael Wetter:<br/>
       Released first version.

is not closed by </li>. Somehow this is not caught by tidylib?

  • The computation of the velocity v is in my opinion obsolete and leads to many divisions by parameters since v[i]=dV_flow[i]/dA. Some other variables like vAB, VAB_flow, dpAB, v_top, etc can either be inlined or removed such that fewer variables are saved.

Is there an interest to 'fix' these things? Otherwise this is not high up on my priority list and then we can deal with this later.

@Mathadon Mathadon self-assigned this Jun 5, 2018
@thorade
Copy link
Member

thorade commented Jun 5, 2018

is not closed by </li>. Somehow this is not caught by tidylib?

This is fixed in later releases of tidy:
htacg/tidy-html5#521

@thorade thorade closed this as completed Jun 5, 2018
@thorade thorade reopened this Jun 5, 2018
@mwetter
Copy link
Contributor

mwetter commented Jun 6, 2018

  • Removing v is fine; I added it so that one can see how big a flow one has through an opening. For this purpose, I think it is convenient to have. Maybe we can reformulate the division as a multiplication, the cost for this is probably trivial.
  • nCom = 2 seems very small, no? We could reduce it but better make some tests to make sure we have a reasonable number.
  • Making some variables protected would make sense. We anyway will have non-backward compatible changes from other issues, so this would be a good time.
  • I added the error control as otherwise, the volume flow rates had large errors. We could look at the states of the control volume to see how sensitive they are if the states mExcAB and mExcBA are removed.
  • I am not worried about the small flow at zero pressure difference and zero temperature difference, as in real applications, there is always some diffusion. The velocity is 0.001 m/s, which should not introduce a big error. But maybe in the meantime, the solvers are good enough to deal with zero flow (this model originates from before having the stream operators).
  • hA is the height of reference pressure zone A, which is zA in Figure 2 in http://simulationresearch.lbl.gov/wetter/download/Wetter-airflow-2006.pdf
  • I am not sure I understand your point of "The same non-zero flow implementation implies that the flow in a port never reverses." All ports are at the same height, the reason for the two flow paths is that one is for flow from left to right, the other for flow from right to left. If there is only a flow from left to right, then the other should simply be (close to) zero.

@Mathadon
Copy link
Member Author

Mathadon commented Jun 6, 2018

Thanks for the feedback!

  • You have a point with the diffusion, but right now the model is not symmetric such that m1_flow does not equal m2_flow when Delta T=0 so that is something that we should probably fix. I think that solvers should indeed be able to deal with zero flow now.

  • Regarding nCom=2, I did a short derivation to estimate the error that we make here for a case with nCom=i and a case with nCom=2i. Assuming that we have equal zone pressures, hA=hB, a quadratic relation with the flow and a pressure difference that rises linearly, then the volume flow rate V is proportional to:

i: V~sqrt(deltah)
2i: V~sqrt(deltah/2)/2 + sqrt(3*deltah/2)/2 = 0.9659258262890682 * sqrt(deltah)

So the error that we make is indeed about 4 %. Going from i to 4i would then imply an error of 4% + 0.016%?

This makes me wonder whether we cannot use a similar derivation to compute a correction coefficient that can be multiplied with a single discretisation to obtain the same results.. The above derivation however does not consider the most general case where zone A and B can have different pressures so it would have to be extended.

Probably best to park this for now due to time constraints but I'd like to hear any thoughts on this kind of approach.

  • Regarding the last point: To me it would make more sense that the highest ports can be interpreted as a lumped node at the top of the door and the bottom ports as a lumped node at the bottom of the door. In this case it would make sense that the flow could reverse, corresponding to physical reality.

@mwetter
Copy link
Contributor

mwetter commented Jun 7, 2018

@Mathadon I removed the + VZer_flow/nCom term and refactored the model to make sure it is symmetric. The changes are backwards compatible. Please see branch issue937_door

A larger refactoring, if there is efficiency to be gained, would need to be done at a later time as I am tied up with some deadlines.

Regarding

To me it would make more sense that the highest ports can be interpreted as a lumped node at the top of the door and the bottom ports as a lumped node at the bottom of the door.

I don't know if this is a good idea. What do you do if there is only flow in one direction through the open door, and how would you make the switch in flow direction differentiable? The model has now variables vTop and vBot that show the velocity and hence the flow direction in the top segment.

mwetter added a commit that referenced this issue Jun 7, 2018
Refactored door and updated reference results for #937
mwetter added a commit that referenced this issue Sep 13, 2018
@mwetter
Copy link
Contributor

mwetter commented Sep 13, 2018

@Mathadon This model has still problems: The two flow rates shown below should be non-zero. Due to the regularization, VBA_flow is negative.
image
The figure is from

simulateModel("IBPSA.Airflow.Multizone.Examples.ReverseBuoyancy", stopTime=3600, numberOfIntervals=5000, method="dassl", tolerance=1e-06, resultFile="ReverseBuoyancy");

I added a patch for this in fdf1f70 (by setting allowFlowReveral=true) but this model needs more revisions as the intent is to have only one-directional flow in each flow path.
As the current version violates this max, and Dymola 2019FD01 beta1 does not accept this violation, I suggest we merge this patch into the master.

Mathadon added a commit that referenced this issue Sep 15, 2018
Set allowFlowReversal to true. For #937
@Mathadon
Copy link
Member Author

looks like this can be closed

@mwetter
Copy link
Contributor

mwetter commented Sep 15, 2018

Reopened so that we don't forget to correct what is explained in #937 (comment)

@mwetter mwetter reopened this Sep 15, 2018
@Mathadon
Copy link
Member Author

@mwetter can this be closed?

@mwetter mwetter closed this as completed Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants