Add diag.swift with tests diag_test.swift #44
Conversation
Sources/diag.swift
Outdated
| /// and zeros everywhere else. | ||
| /// | ||
| /// - Parameter v : A vector data structure |
Sources/diag.swift
Outdated
| let diagonalStartIndex = k >= 0 ? k : length * abs(k) | ||
|
|
||
| let vectorIndices = (0..<v.count).map {index in |
There was a problem hiding this comment.
My one thought is by precomputing these, we allocate a n size array when we really only need each vector index once. We could pull the data assignment from the for loop below into this loop (and change this loop from a map to a for) and avoid this. Seems like it doesn't matter much though.
Sources/diag.swift
Outdated
| public func diag(A: Matrix<Double>, k: Int) -> Vector<Double> | ||
| { | ||
| precondition(A.rows == A.columns, "Matrix must be square") |
There was a problem hiding this comment.
I think we eventually want to move away from using a raw precondition or assert to use the wrappers warn and error. I was hoping that the wrappers would help in testing but I kind of got stuck. We don't have to change this now, but perhaps it's a good time to have a conversation about this; I'd love to pick your brain
Sources/diag.swift
Outdated
| { | ||
| precondition(A.rows == A.columns, "Matrix must be square") | ||
| precondition(k < abs(A.rows), "abs(k) must be less than the the A dimensions") |
There was a problem hiding this comment.
I believe the matrix verifies rows are always positive so the abs is unnecessary
Sources/diag.swift
Outdated
| data.removeSubrange(rangeLower) | ||
|
|
||
| let indices = data.indices.filter {$0 % interval == 0} |
There was a problem hiding this comment.
Clever! The only concern is that according to the removeSubrange docs: the complexity is "O(n), where n is the length of the collection." So this it's potentially expensive to slice off the ends. What if instead in the filter we add on a bounds check that the element is between the start and end index? And I guess we'd have to add an offset for the mod operation too...
Tests/NiftyTests/diag_test.swift
Outdated
| let tests = | ||
| [ | ||
| testCase([("testBasic", self.testBasic)]), | ||
| ] |
There was a problem hiding this comment.
Looks like some whitespace weirdness
Tests/NiftyTests/diag_test.swift
Outdated
| // TODO: fill me in | ||
| print("\n\t*** WARNING: Test unimplemented - \(#file)\n") | ||
|
|
||
| func testBasic() |
There was a problem hiding this comment.
Very nice! Some time let's talk about strategies for testing, whether the testBasic pattern I started with is really what we want to do or whether smaller test cases are better.
|
Very clean and readable! Nice! Left a few comments. We don't have to change anything necessarily but let's discuss before it goes in |
|
Great feedback! I'll have a look at it in the weekend, and try to avoid the obvious typos next time. |
|
Well it was very nice code to review! As far as discussions go I think discussions specific to a ticket make sense in tracker. But I think both these questions are larger so slack makes sense? What do you think? |
|
I think we can do the broad discussions in Slack (make some more channels?) and then make some summaries/conclusions in Tracker afterwards? |
Description of Changes
Completed diag.swift and made tests for diag_test.swift.
Resolves
Adds to core functionality.
https://www.pivotaltracker.com/story/show/159221978
Todos
Author:
Reviewer: