fortran-lang / minpack

Modernized Minpack: for solving nonlinear equations and nonlinear least squares problems
https://fortran-lang.github.io/minpack/
Other
94 stars 20 forks source link

Exit early to avoid waterfall code #46

Open ivan-pi opened 2 years ago

ivan-pi commented 2 years ago

From Recommendations To Write (Slightly More) Readable And (Thus) Robust Code 7:

Exit early to avoid waterfall code. Especially for longer code blocks, this minimizes cognitive overhead.

(originally a suggestion from @interkosmos posted at Discourse)

The current modularized minpack source code, includes sections for checking the input parameters like this one taken from lmder1:

        Info = 0

        ! check the input parameters for errors.

        if (n > 0 .and. m >= n .and. Ldfjac >= m .and. Tol >= zero .and. &
            Lwa >= 5*n + m) then
            ! call lmder.
            maxfev = 100*(n + 1)
            ftol = Tol
            xtol = Tol
            gtol = zero
            mode = 1
            nprint = 0
            call lmder(fcn, m, n, x, Fvec, Fjac, Ldfjac, ftol, xtol, gtol, maxfev,   &
                     & Wa(1), mode, factor, nprint, Info, nfev, njev, Ipvt, Wa(n + 1)&
                     & , Wa(2*n + 1), Wa(3*n + 1), Wa(4*n + 1), Wa(5*n + 1))
            if (Info == 8) Info = 4
        end if

Here in the "easy" driver there is only level of indentation, so it's not as problematic, but for the actual routines which do the work, getting rid of one level of indentation would make them easier on the eye.

The solution is to return early, per the advice above:

info = 0  ! flag for improper input parameters

! check the input parameters for errors

if (n < 1 .or. m < n) return
if (ldfjac < m) return
if (tol < zero) return
if (lwa < 5*n + m) return

! default settings

maxfev = 100*(n+1)
ftol = tol
xtol = tol
gtol = zero
mode = 1
nprint = 0

call lmder( ... )
if (info == 8) info = 4

Since Fortran does not short-circuit expressions I guess it also makes sense to separate the condition. (For correct input parameters, all the conditionals should be false, but in the case not, we might save a few evaluations and exit early.) This might also just by a personal preference of mine, that is I dislike long logical expressions and/or line continuation:

if (n < 1 .or. m < n .or. ldfjac < m .or. tol < zero .or. lwa < 5*n + m) &
    return
awvwgk commented 2 years ago

Maybe we should also make the return code more concrete and have info point to the incorrect argument rather than just raising a general input error (similar like done in Lapack).

ivan-pi commented 2 years ago

I'm generally in favor of making the error codes more specific. The only downside I see is this will likely "invalidate" the current usage instructions and also won't necessarily be back compatible, requiring consumers to modify their programs; essentially we'd be breaking the API.

Any breaking changes would have to be communicated clearly.

ivan-pi commented 2 years ago

We could introduce a compile time DEBUG flag (via the preprocessor), which would produce verbose error output if enabled. This would not break the established error codes.