libprima / PRIMA.jl

a Julia interface to PRIMA, a Reference Implementation for Powell's methods with Modernization and Amelioration
MIT License
21 stars 5 forks source link

check_bounds in tests/runtests can be written more compactly #24

Closed dmbates closed 9 months ago

dmbates commented 9 months ago

This is a very minor stylistic point but the check_bounds function in test/runtests.jl

        function check_bounds(xl, x, xu)
            flag = true
            for (lᵢ, xᵢ, uᵢ) in zip(xl, x, xu)
                flag &= lᵢ ≤ xᵢ ≤ uᵢ
            end
            return flag
        end

could be written

any(xl .≤ x .≤ xu)

These are equivalent except that the any version will short-circuit as soon as it finds a bounds violation. In other words, it is equivalent to

function check_bounds(xl, x, xu)
    for (l, x, u) in zip(xl, x, xu)
        l ≤ x ≤ u || return false
    end
    return true
end

Perhaps more important is that zip doesn't fail if the lengths of xl, x, and xu don't all match; it uses the shortest length. The all version throws DimensionMismatch in such a case.

julia> collect(zip(ones(4), zeros(3)))
3-element Vector{Tuple{Float64, Float64}}:
 (1.0, 0.0)
 (1.0, 0.0)
 (1.0, 0.0)

julia> all(zeros(4) .≤ rand(4) .≤ ones(3))
ERROR: DimensionMismatch: arrays could not be broadcast to a common size; got a dimension with lengths 4 and 3
zaikunzhang commented 9 months ago

any(xl .≤ x .≤ xu)

I much prefer this style.

emmt commented 9 months ago

You mean:

all(xl .≤ x .≤ xu)

not any(xl .≤ x .≤ xu).

You have to realize that:

  1. all(xl .≤ x .≤ xu) implies creating, at least, one temporary array while check_bounds avoids that.
  2. check_bounds , which is supposed to return true, avoids branching to allow for loop-vectorization. This is not the case of all, any, etc.
  3. check_bounds can be modified to check the sizes.

Nevertheless, such optimizations are irrelevant for test code and I have modified check_bounds as suggested in commit 2c756f3203ee5870725d5aad0002a4ce121be6da. Thanks for the suggestion.