Surge icon indicating copy to clipboard operation
Surge copied to clipboard

Deprecation/Removal of `sqrt` variant with custom output storage?

Open regexident opened this issue 5 years ago • 1 comments

tl;dr

A pair of functions seems to have been marked as public at some point in the past, that should have remained internal and have since been obsoleted. We should remove them from the public API.

What

Surge contains a somewhat odd function for calculating sqrt by providing a custom output buffer:

/// Elemen-wise square root with custom output storage.
///
/// - Warning: does not support memory stride (assumes stride is 1).
public func sqrt<MI: UnsafeMemoryAccessible, MO: UnsafeMutableMemoryAccessible>(_ lhs: MI, into results: inout MO) where MI.Element == Float, MO.Element == Float {
    return lhs.withUnsafeMemory { lhsMemory in
        results.withUnsafeMutableMemory { rm in
            precondition(lhsMemory.stride == 1 && rm.stride == 1, "sqrt doesn't support step values other than 1")
            precondition(rm.count >= lhsMemory.count, "`results` doesnt have enough capacity to store the results")
            vvsqrtf(rm.pointer, lhsMemory.pointer, [numericCast(lhsMemory.count)])
        }
    }
}

(Source)

Why

This pattern is nowhere else to be found in the framework and it looks to me like it was intended primarily for internal use as a base-implementation of the non-custom variant and possibly marked as public by accident(?):

/// Elemen-wise square root.
///
/// - Warning: does not support memory stride (assumes stride is 1).
public func sqrt<C: UnsafeMemoryAccessible>(_ lhs: C) -> [Double] where C.Element == Double {
    var results = [Double](repeating: 0.0, count: numericCast(lhs.count))
    sqrt(lhs, into: &results)
    return results
}

(Source)

(The method in question was added in this commit.)

How

As such I'd like to nominate both variants (Float & Double) of this function for deprecation with the next minor release, with removal on next major release.

What do you guys think, @mattt, @alejandro-isaza?


Update: with #119 merged we now have a proper sqrtInPlace function for efficient non-allocating computation of sqrt:

func sqrtInPlace<C: UnsafeMutableMemoryAccessible>(_ lhs: inout C) where C.Element == Float {
    var elementCount: Int32 = numericCast(lhs.count)
    lhs.withUnsafeMutableMemory { lm in
        precondition(lm.stride == 1, "\(#function) doesn't support step values other than 1")
        vvsqrtf(lm.pointer, lm.pointer, &elementCount)
    }
}

(Source)

regexident avatar Sep 24 '19 16:09 regexident

That sounds great, @regexident. I can't speak to the original implementation, but I agree with your proposal to deprecate and remove those functions.

mattt avatar Sep 24 '19 16:09 mattt