Unit consistency for s-parameter in Modelica.Fluid.Vessels.PartialLumpedVessel #4398
Conversation
It will be deduced to be of type mass-flow-rate ("kg/s") not "1". There are multiple ways of doing it, but I thought this was the less intrusive. |
|
I am struggeling with the (deduced) unit of |
Ah, yes, I messed up. Yes, it will be length-based i.e. "m", based on the other branch and for the mass-flow branch we do: The alternative would be something like: |
|
What about |
There are multiple alternatives here, but this seemed like the smallest change. Compare to Modelica.Electrical.Analog.Interfaces.IdealSemiconductor
WalkthroughThis pull request adds two constant parameters— Changes
Sequence Diagram(s)sequenceDiagram
participant IE as Inflow Event
participant OT as OpenTank Model
participant CP as Calculation Process
IE->>OT: Trigger inflow condition
OT->>CP: Retrieve ports[i].m_flow, unitHeight, unitMassFlow
CP-->>OT: Compute s[i] = ports[i].m_flow * (unitHeight/unitMassFlow)
OT->>IE: Update state variable s[i]
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Modelica/Fluid/Vessels.mo (1)
257-258: Consider an explicit type declaration for the s parameter.While the current changes correctly address the unit consistency issue with
s, you might want to consider Beutlich's suggestion to explicitly declareswith a specific unit type:-Real[nPorts] s(each start = fluidLevel_max) +"Curve parameters for port flows vs. port pressures; for further details see, Modelica Tutorial: Ideal switching devices"; +SI.Length[nPorts] s(each start = fluidLevel_max) "Curve parameters for port flows vs. port pressures; for further details see, Modelica Tutorial: Ideal switching devices";This would make the intended unit type more explicit and potentially avoid similar issues in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Modelica/Fluid/Vessels.mo(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: external_c_checks_cmake_windows-msvc
- GitHub Check: external_c_checks_cmake_windows-cygwin
🔇 Additional comments (3)
Modelica/Fluid/Vessels.mo (3)
271-271: Improved indentation in the protected section.The indentation of the "protected" section has been corrected to match the style guide, making it consistent with the class indentation level.
295-296: Good addition of unit conversion constants.These constants provide appropriate unit conversion factors to ensure consistency in the calculation of the
sparameter. By setting both to 1 with appropriate units (unitMassFlowas mass flow rate andunitHeightas length), you provide the necessary dimensional conversion while maintaining numeric values. TheHideResult=trueannotation appropriately prevents these implementation details from appearing in simulation results.
379-379: Correctly implemented unit consistency for s-parameter.This change addresses the unit consistency issue for the
sparameter when there is inflow. Previously,s[i]would have been assigned a mass flow rate (kg/s), but now it's being scaled to have units of length (m) by multiplying by the ratio ofunitHeighttounitMassFlow. This ensures thats[i]consistently has the same unit (length) across all branches of the conditional statement:
- In regular flow:
s[i] = fluidLevel - portsData_height[i](length)- In inflow:
s[i] = ports[i].m_flow*(unitHeight/unitMassFlow)(now length)- In no flow:
s[i] = (ports[i].p - vessel_ps_static[i])/Medium.p_default*(portsData_height[i] - fluidLevel)(length)This change eliminates the unit inconsistency identified in the PR discussion.
There are multiple alternatives here, but this seemed like the smallest change.
Compare to Modelica.Electrical.Analog.Interfaces.IdealSemiconductor
The new indentation of "protected" is consistent with style-guide, previously it was indented less than the class.
Summary by CodeRabbit
New Features
Refactor