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

Refactor check valve with spline interpolation #1938

Merged
merged 12 commits into from
Oct 22, 2024
Merged

Conversation

AntoineGautier
Copy link
Contributor

This addresses #1937.

@AntoineGautier
Copy link
Contributor Author

@mwetter This is ready. Can you suggest a name for an external review?

AntoineGautier and others added 3 commits October 15, 2024 17:41
This sentence is wrong as it refers to a velocity and not a flow rate. It is also not clear what a value would be at other flow rates, hence I deleted it
Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoineGautier : I am not sure we approached this the right way, in part because I don't understand your implementation (see inline comment).

Wouldn't it be simpler if we were to compute $dp$ from $\dot m$? This would make it simpler because both components have the same $\dot m$, so we can simply add their resistances. For the fixed resistance the evaluation is easy (with the flow function). For the actual valve, we can make a linear behavior for $\dot m \le 0$ and quadratic for $\dot m \ge \dot m_0/2$. In between we use a polynominal that fits the first and second derivatives at these points. The coefficients of the polynomial can be computed as parameter expressions.

IBPSA/Fluid/FixedResistances/CheckValve.mo Outdated Show resolved Hide resolved
@AntoineGautier
Copy link
Contributor Author

AntoineGautier commented Oct 21, 2024

@mwetter With 11097a8 the function calls are now included in the if else clauses, min and max are removed and parameters are used where possible.
I've also updated the documentation for the derivatives of the basic flow functions as discussed. Note the important information that these derivatives are formulated under the assumption of a constant flow coefficient. This is consistent with the annotation derivative(zeroDerivative=k) in basicFlowFunction_dp. This also means that if a model features a varying flow coefficient (e.g., any actuator model), these derivative functions will not be used by the tools, which will rely on automatic differentiation instead.

Copy link
Contributor

@mwetter mwetter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AntoineGautier : Thanks for the changes. This addresses my concerns that we discussed on Friday.

@mwetter mwetter enabled auto-merge October 21, 2024 20:08
@mwetter mwetter merged commit 8ed71ca into master Oct 22, 2024
3 checks passed
@mwetter mwetter deleted the issue1937_checkValve branch October 22, 2024 06:49
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

Successfully merging this pull request may close these issues.

2 participants