ModelicaStandardLibrary icon indicating copy to clipboard operation
ModelicaStandardLibrary copied to clipboard

AdditionalSqrt

Open HansOlsson opened this issue 1 year ago • 6 comments
trafficstars

Follow-up to #4358 Making it a draft PR in case there is more missing.

HansOlsson avatar Apr 23 '24 15:04 HansOlsson

Here are some regexp search results, including matches both in code and documentation:

  • \^[ ]*[-]?[ ]*0.5 : 58 matches
  • \^[ ]*[-]?[ ]*\([ ]*1[ ]*/[ ]*2[ ]*\) : 18 matches

Many of these are not in documentation, and I'm wondering if there could be any cases where raising to 0.5 is more correct than using sqrt?

henrikt-ma avatar Apr 25 '24 07:04 henrikt-ma

Will recheck. For the 2nd regexp in terms of unit-checking most don't seem to matter:

  • Modelica.Media.Water.IF97_Utilities.BaseIF97.Transport.cond_dTp: lambdaREL0 := TREL^(1/2)*sum(a[i]*TREL^(i - 1) for i in 1:4); Where TREL has unit 1
  • Modelica.Thermal.HeatTransfer.Components.Convection documentation Nu = 0.453*Re^(1/2)*Pr^(1/3);
  • Modelica.Media.IdealGases.Common.MixtureGasNasa.gasMixtureViscosity (8*(1 + M[i]/M[j]))^(1/2)
  • Modelica.Fluid.Dissipation.Utilities.SharedDocumentation.HeatTransfer.Channel.kc_evenGapLaminar Nu_3 = [2/(1+22*Pr)]^(1/6)*(Re*Pr*d_hyd/L)^(1/2) documentation-class
  • Modelica.Fluid.Dissipation.HeatTransfer.StraightPipe.kc_laminar_KC(Re*IN_con.d_hyd/IN_con.L)^(1/2)- which has unit 1.

The following seem more concerning - but involve other exponents as well so should probably be a separate PR:

  • Modelica.Media.IdealGases.Common.MixtureGasNasa.lowPressureThermalConductivity but there are lots of other unit-issues as well including other exponents.
  • Modelica.Media.IdealGases.Common.MixtureGasNasa.mixtureViscosityChung all kinds of exponents

HansOlsson avatar Apr 25 '24 07:04 HansOlsson

Have now gone through all of them, the remaining ones are:

  • Documentation
  • Part of class with other non-integer exponents as well. To me the minor benefit of having sqrt(x) instead of x^0.5 is outweighed by the confusion caused by mixing sqrt(x) and x^(1/3)

HansOlsson avatar Apr 30 '24 12:04 HansOlsson

@HansOlsson would you suggest some other reviewers to get this through?

casella avatar Jun 22 '24 16:06 casella

@HansOlsson would you suggest some other reviewers to get this through?

I believe you can review it, as some are part of media and possibly @AHaumer as it relates to complex numbers.

HansOlsson avatar Jun 23 '24 18:06 HansOlsson

I have no strong opinion about this, I need to think about it.

casella avatar Jul 02 '24 21:07 casella

I have no strong opinion about this, I need to think about it.

In general sqrt(x) should be preferred over x^0.5 (assuming both are directly mapped to the corresponding C-expressions):

  • If x^0.5 is mapped to pow(x,0.5), then sqrt(x) is a specialized algorithm that is more efficient, might have better precision, and most CPUs have even dedicated square root instructions.
  • Readability is better - it is much clearer that the computation is a square root computation

MartinOtter avatar Jul 05 '24 10:07 MartinOtter

@casella Can this be backported to maint/4.1.0? No milestone either.

Esther-Devakirubai avatar Jul 19 '24 07:07 Esther-Devakirubai

@casella Can this be backported to maint/4.1.0? No milestone either.

Given @HansOlsson's comment, you can do that. It's not critical but it doesn't harm.

casella avatar Jul 31 '24 09:07 casella

Backporting this to maintenance branch by #4452

Esther-Devakirubai avatar Aug 16 '24 03:08 Esther-Devakirubai