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
optimize Fluid.BaseClasses.FlowModels.basicFlowFunction_dp #279
Comments
I'm in favour of this change since the combined functions are indeed very complex and often they require many evaluations because they are used in non-linear systems. Moreover a linear regime would better correspond to the laminar flow regime that occurs in practice. I suggest to look for a solution in this direction. For valves things may be a bit more complicated, although a pragmatic approximation may be sufficiently accurate? |
I created an (arguably dumb) implementation for
I then evaluated before
after
so this does indicate that a lot of performance can be gained. We should however look for a better formulation. Can't we construct some non-piecewise function that approaches edit: This is somewhat more elegant:
we could tweak the switching point between the sinusoid and the square root to have a continuous first derivative. This implementation should work for valves too. |
Note that these functions are not inlined even though |
The functions are inlined when
The speed is then:
|
These are nice speed-ups. It would be good to investigate how expensive evaluating You also need to use |
Based on the following example
I find: |
The way I understood it the The code contains the
|
Code example is on https://github.com/iea-annex60/modelica-annex60/tree/issue279_flowFunctions Also, I have the impression that the old implementation does not comply with the Modelica specification:
This is currently not the case in the regularisation region as illustrated by |
Here is an alternative that has a continuous first derivative:
computation time is marginally faster than when using the sinusoid function. A possible problem is that this function's inverse is pretty expensive to evaluate:
a solution may be to allow that f(dp) is not the exact inverse of f(m_flow) in the turbulent region as is currently the case. |
Some more analyses: implementation for m_flow
or even:
When running the first option together with the proposed implementation of InliningSetting This causes reference results to change since in some models the function inverse will not be used, where this was the case earlier. Since the normal function and its inverse are not identical, this will change the reference results. I'm happy with this solution, so I'll already make a pull request. Edit: My building model's speed increased by ~ 11% due to this measure. |
Looks like a good improvement from @Mathadon , looking forward to discuss this on the call later today. |
Also, I tested this new flow function for the pipes example in #264 and this implementation seems to solve the problem we were experiencing with different mass flow rates for 2 x 50 m pipes vs. 1 x 100 m pipe. |
@marcusfuchs will review, ensure that it is twice continuously differentiable and add a unit test for the 1st and 2nd derivative. |
I added a test case for the 1st- and 2nd-order derivative each. The 1st-order derivative check works for from_dp = true and from_dp = false. The 2nd-order derivative check works for from_dp = true but fails for from_dp = false. So if the tests are implemented correctly, the flowFunction_dp would be twice differentiable, the flowFunction_mflow only once. I will double-check this tomorrow. |
Also the 2nd-order derivative check works with from_dp = false on the dassl solver if the solver tolerance is decreased from 1e-4 to 1e-6. |
@marcusfuchs import buildingspy.development.refactor as r
r.move_class("Annex60.Fluid.BaseClasses.FlowModels.Examples.InverseFlowFunction",
"Annex60.Fluid.BaseClasses.FlowModels.Validation.InverseFlowFunctions")
r.move_class("Annex60.Fluid.BaseClasses.FlowModels.Examples.TestFlowFunctions_1st_derivative",
"Annex60.Fluid.BaseClasses.FlowModels.Validation.FirstDerivative")
r.move_class("Annex60.Fluid.BaseClasses.FlowModels.Examples.TestFlowFunctions_2nd_derivative",
"Annex60.Fluid.BaseClasses.FlowModels.Validation.SecondDerivative")
r.move_class("Annex60.Fluid.BaseClasses.FlowModels.Examples.TestFlowFunctions",
"Annex60.Fluid.BaseClasses.FlowModels.Validation.FlowFunctions") as they are validation tests and not examples. I also added scripts for the regression tests. |
Currently we are again experiencing different mass flows during laminar/transient flow regimes for two 50 m pipes in series compared to a single pipe of 100 m, whereas they should be the same. Please refer to bramvdh91#24 for an example of the issue. The problem only appears when |
The function
Fluid.BaseClasses.FlowModels.basicFlowFunction_dp
andbasicFlowFunction_m_flow
lead to many operations outside the turbulent region.This issue should look at whether a simpler implementation leads to faster computation. One could for example use a linear function for |m_flow| < m_flow_turbulent/2 and use regularization between these two regions. This regularization could be done with a 3rd order polynomial. For fixed resistances, all polynomial coefficients are parameters, so there should be little overhead, whereas for valves, these coefficients need to be computed during the time stepping. Hence, maybe extending the linear function to
|m_flow| < 2/3*m_flow_turbulent may be more efficient.
The text was updated successfully, but these errors were encountered: