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

Deprecation/Removal of sqrt variant with custom output storage? #121

Open
regexident opened this issue Sep 24, 2019 · 1 comment
Open

Deprecation/Removal of sqrt variant with custom output storage? #121

regexident opened this issue Sep 24, 2019 · 1 comment

Comments

@regexident
Copy link
Collaborator

regexident commented Sep 24, 2019

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)

@mattt
Copy link
Collaborator

mattt commented Sep 24, 2019

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

@regexident regexident mentioned this issue Oct 13, 2019
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants