dp_tau_centrifugal bug fix Reynolds number

[RaphaelGebhart]

Continues #168.

In the centrifugal pump model used in
ThermofluidStream.Processes.Internal.TurboComponent.dp_tau_centrifugal,
the Reynolds number was calculated incorrectly.

The Reynolds number was overestimated by a factor equal to the density in kg/m³
(e.g. a factor of ~1000 for water). This led to overly strong viscosity effects in the
pump model and therefore incorrect results.

All test models and examples were checked and updated accordingly.
The bug fix has a major impact for open-loop systems, but only a minor impact for
closed-loop systems, when considering the input output behaviour.

Please vote on how to proceed:

• 1) Apply the bug fix (similar to Fix faulty limiting of Reynolds number #253) (Result of the vote)
• 2) Do not apply the bug fix

If you vote for 2), please indicate which option you prefer:

• 2.1)
  First step: mark the function as obsolete
  Second step: remove it from the choices of
  replaceable function dp_tau_pump in ThermofluidStream.Processes.Pump
  Third step: remove the function completely

• 2.2)
  Do nothing

Results for old vs new (bug fixed) correlation:

[dzimmer]

Sure. A mistake should be corrected.
If the impact is major (as it seems), you may want to sync it with a major version release.

[RaphaelGebhart]

I (with support of my trusted AI tool) suggest the following migration strategy:

v1.3

• Introduce parameter Boolean useLegacyVersion = true
• Add an assertion:
  assert(not useLegacyVersion, "...", AssertionLevel.warning);
• This means users will receive a warning by default, informing them that the legacy behavior is deprecated. They are asked to switch to the new version and told that the legacy version will be disabled in v2.0.

v1.4

• Introduce parameter AssertionLevel assertionLevel = AssertionLevel.error
• By default, the assertion is promoted to an error, forcing users to migrate.
• For switching from legacy behavior to the new version users can downgrade the assertion level back to a warning.

v2.0

• Remove useLegacyVersion and assertionLevel
• Remove the legacy implementation entirely
• Provide a conversion script that removes all related modifiers from user models

@dzimmer do you approve this approach?

[CorentinLepais]

I would also fix the bug. You may also want to display the warning on the icon layer to make sure it is properly identified by the users

[nieweber]

I would suggest to fix it. If we want to add the "legacy-flag", I'd only suggest it if the next release is no major release. If we do a major release, we will put it to the release notes and then it should be fine. As long as we don't create a new release it is up to the user to use the latest release or the latest main. So my suggestion: fix it and prepare a major release.

[RaphaelGebhart]

I would also fix the bug. You may also want to display the warning on the icon layer to make sure it is properly identified by the users

Thanks for the suggestion. Unfortunately, I don’t think this is feasible in this case.

The bug is located inside a function that is used by the pump model, and the proposed fix is also entirely contained within that function. The pump model itself can be configured to use different characteristic functions.

Displaying a warning on the icon layer would require the model to receive information from the selected function. This, in turn, would require introducing an additional output to the function interface. Adding such an output would be a breaking API change and therefore only acceptable in a major release. (If I'm not mistaken)

[RaphaelGebhart]

I would suggest to fix it. If we want to add the "legacy-flag", I'd only suggest it if the next release is no major release. If we do a major release, we will put it to the release notes and then it should be fine. As long as we don't create a new release it is up to the user to use the latest release or the latest main. So my suggestion: fix it and prepare a major release.

The reason I prefer the intermediate step is that it allows us to update all models already in v1.3 and ensure that they run correctly with the fix, while still retaining the option to fall back to the old behavior.

If we apply the fix only in v2.0, we lose access to the legacy formulation entirely, which makes migration harder.
In fact, both ThermofluidStream.Examples.SimpleCoolingCycle and ThermofluidStream.Processes.Tests.Pump failed when applying the fix. While the required model changes were relatively straightforward, this was only obvious because the issue was known.

Without an explicit warning, users may experience failing models without a clear indication of the root cause, which could significantly increase debugging time. The intermediate step provides a controlled migration path and makes the transition much more transparent for users.

[fvanderlinden]

I suggest going with option 1.3 first, than update with a major release to 2.0.

Keeping backward compatibility is important. We will break models with a direct fix.

[RaphaelGebhart]

Ready for review @CorentinLepais. We also have to update the reference files for regression testing in this case.

[CorentinLepais]

Optional
Since you have already made some changes to the test model, can you maybe improve the layout (extending the size of the white area, add proper names and maybe two showValue block for the two outputs).

[CorentinLepais]

Maybe the documentation of the pump model can be updated to mention the legacy Reynold option (e.g. "- Centrifugal pump, which implements the equations of a scalable centrifugal pump. Warning the function uses by default a deprecated Reynold formulation which can lead to errors. Please see dp_tau_centrifugal documentation for more information")

[CorentinLepais]

• Icon display in package browser not homogenous (icon layer size is currently based on graphic layer)
• Documentation missing (at least explain the purpose of the model)
• Short description next to model name missing
• Mirror horizontally conduction element

The controller does not allow to reach the setpoints (maybe because the heat source is too small) and when simulated on a longer time the loop starts to oscillate.

[CorentinLepais]

• Icon display in package browser not homogenous (icon layer size is currently based on graphic layer)
• Documentation missing (at least explain the purpose of the model)
• Short description next to model name missing
• Mirror horizontally conduction element

Controller does not allow to reach the setpoint (maybe heat load to small). But maybe it is intended or at least not an issue

[CorentinLepais]

• Icon display in package browser not homogenous (icon layer size is currently based on graphic layer)
• Documentation missing (at least explain the purpose of the model)
• Short description next to model name missing
• Mirror horizontally conduction element
• inlet_temp block not connected

[CorentinLepais]

• Icon display in package browser not homogenous (icon layer size is currently based on graphic layer)
• Documentation missing (at least explain the purpose of the model)
• Short description next to model name missing
• Mirror horizontally conduction element
• Clean connection lines
• inlet_temp block not connected

[CorentinLepais]

• Icon display in package browser not homogenous (icon layer size is currently based on graphic layer)
• Documentation missing (at least explain the purpose of the model)
• Short description next to model name missing
• Mirror horizontally conduction element
• inlet_temp block not connected