Skip to content

Conversation

@beutlich
Copy link
Member

Alternative PR for #3499 to address #2969.

@beutlich beutlich added the L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined label Mar 18, 2020
@beutlich beutlich added this to the MSL4.0.0 milestone Mar 18, 2020
@beutlich beutlich self-assigned this Mar 18, 2020
@HansOlsson
Copy link
Contributor

Does anyone know if we have another test for the underlying model/function?
(I was planning to run coverage later today, so I guess I can tell then.)

Copy link
Contributor

@maltelenz maltelenz left a comment

Choose a reason for hiding this comment

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

In absence of a response from the library officer, I think this is our best option.

@HansOlsson
Copy link
Contributor

HansOlsson commented Mar 18, 2020

This does not seem ideal.

As far as I can see Modelica.Fluid.Dissipation.HeatTransfer.HeatExchanger.kc_roundTube is only used in this example ModelicaTest.Fluid.Dissipation.Verifications.HeatTransfer.HeatExchanger.kc_roundTube

However,

  • Also Nu[2] is problematic, but around 0.00244 s. Thus the problem is with SlitFin and LouverFin
  • The model is not a straightforward function-test since it implicitly solves for mass-flow based on Reynolds number, but that is not the cause of the problem.
  • ModelicaTest.Fluid.Dissipation.Verifications.HeatTransfer.HeatExchanger.kc_roundTube_KC tests the underlying function, and in that test-model variant 2 and 3 are commented out (i.e. SlitFin and LouverFin). I would propose to do the same here.

@beutlich
Copy link
Member Author

Closed in favour of #3512.

@beutlich beutlich closed this Mar 18, 2020
@beutlich beutlich deleted the remove-test-model branch March 18, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L: ModelicaTest Issue addresses ModelicaTest, ModelicaTestConversion4 or ModelicaTestOverdetermined wontfix Issue that will not be fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants