Skip to content

transform: performance like it matters - #283

Merged
TimothyStiles merged 8 commits into
bebop:mainfrom
soypat:poly-perf
Nov 29, 2022
Merged

transform: performance like it matters#283
TimothyStiles merged 8 commits into
bebop:mainfrom
soypat:poly-perf

Conversation

@soypat

@soypat soypat commented Nov 27, 2022

Copy link
Copy Markdown
Contributor

Lot of noise in this PR, sorry about that. What matters is that

  • I'm using a 256byte table for complement calculation instead of map
  • I'm avoiding heap allocations where possible by using a byte slice and unsafe conversion.
// ReverseComplement takes the reverse complement of a sequence.
// ReverseComplement expects ASCII input.
func ReverseComplement(sequence string) string {
    n := len(sequence)
    newSeq := make([]byte, n)
    for i := 0; i < n; i++ {
        newSeq[i] = complementTable[sequence[n-i-1]]
    }
    // This is how strings.Builder works with the String() method. If Mr. Go says it's safe...
    return *(*string)(unsafe.Pointer(&newSeq))
}

@gitpod-io

gitpod-io Bot commented Nov 27, 2022

Copy link
Copy Markdown

@soypat

soypat commented Nov 27, 2022

Copy link
Copy Markdown
Contributor Author

Test is failing because I included a test for the complement table and it seems it was doing something I did not expect. 'U's complement is 'A'?

Edit: Updated tests. I learned that Uracil binds with Adenine in RNA sequences :)

Comment thread transform/examples_test.go
Comment thread transform/transform.go
Comment thread transform/transform.go Outdated
Comment thread transform/transform.go Outdated
Comment thread transform/transform_test.go Outdated
@Koeng101

Copy link
Copy Markdown
Contributor

A few things to change (single letter variables) - mostly interested in benchmarks. How much faster is it?

@TimothyStiles

Copy link
Copy Markdown
Collaborator

I can fix variables and stuff but I'm also interested in benchmarks! Would be happy to start on a real benchmark suite for Poly @soypat.

@soypat

soypat commented Nov 28, 2022

Copy link
Copy Markdown
Contributor Author

@Koeng101 @TimothyStiles I'll fix the variable names and add better documentation today- but I'd like to note that i as a generic counter in small scopes has been shown to be as readable or even more readable than a full length name. See Beniamini et al..

It'd be awesome to have a benchmark suite like you say to get a real taste of how performance changes affects those using the library 😄, more than happy to help out however I can. Been looking at refactoring the fasta package 👀.

Benchmarks for 32byte string input (click)

Before

goos: linux
goarch: amd64
pkg: github.com/TimothyStiles/poly/transform
cpu: Intel(R) Core(TM) i5-8265U CPU @ 1.60GHz
BenchmarkReverseComplement-8   	 1639549	       714.7 ns/op	     224 B/op	       3 allocs/op
BenchmarkComplement-8          	 3199111	       383.4 ns/op	      48 B/op	       1 allocs/op
BenchmarkReverse-8             	 4345659	       325.0 ns/op	     176 B/op	       2 allocs/op
PASS

After

BenchmarkReverseComplement-8   	18890924	        60.14 ns/op	      32 B/op	       1 allocs/op
BenchmarkComplement-8          	25396846	        55.45 ns/op	      32 B/op	       1 allocs/op
BenchmarkReverse-8             	19949305	        55.67 ns/op	      32 B/op	       1 allocs/op

@soypat
soypat marked this pull request as ready for review November 28, 2022 21:47

func TestComplementBaseError(t *testing.T) {
complementBase := ComplementBase('!')
if complementBase != ' ' {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Koeng101 if complement base can't find a complement for the given nucleotide what should it return? N?

Comment thread transform/transform_test.go Outdated
@TimothyStiles
TimothyStiles merged commit 07821a7 into bebop:main Nov 29, 2022
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

Successfully merging this pull request may close these issues.

3 participants