Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit early to avoid waterfall code #46

Open
ivan-pi opened this issue Feb 26, 2022 · 3 comments
Open

Exit early to avoid waterfall code #46

ivan-pi opened this issue Feb 26, 2022 · 3 comments
Labels

Comments

@ivan-pi
Copy link
Member

ivan-pi commented Feb 26, 2022

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
Copy link
Member

awvwgk commented Mar 5, 2022

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
Copy link
Member Author

ivan-pi commented Mar 5, 2022

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
Copy link
Member Author

ivan-pi commented May 22, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants