Implement isAlmostEqual method for floating-point tensors. #348
Conversation
Co-Authored-By: Richard Wei <[email protected]>
|
Can we change the test helpers that test for approximate equality to use this function? Also, isn't |
| to other: Tensor, | ||
| tolerance: Scalar = Scalar.ulpOfOne.squareRoot() | ||
| ) -> Bool { | ||
| return self.shape == other.shape && |
There was a problem hiding this comment.
You can use Tensor.elementsApproximatelyEqual(_:tolerance:) directly to make this faster. Also, maybe we should also add the to argument label to all these other functions that check for element-wise equality.
There was a problem hiding this comment.
Also, maybe we should also add the
toargument label to all these other functions that check for element-wise equality.
This elementsApproximatelyEqual(_:tolerance:) method name is a sentence rather than a verb phrase, and "equal" here is a transitive verb. Same for Sequence.elementsEqual(_:).
We should change "approximately" to "almost" for consistency with isAlmostEqual(to:).
There was a problem hiding this comment.
I've renamed elementsApproximatelyEqual to elementsAlmostEqual and this is now called from isAlmostEqual. For consistency I also changed the condition on Scalar to now conform to TensorFlowFloatingPoint so that it's convertible to Double. Ptal.
This is to follow SE-0259. If that gets changed, we should change ours to follow. |
| XCTAssertTrue(x.isAlmostEqual(to: y, tolerance: 0.01)) | ||
| } | ||
|
|
||
| func testIsNotAlmostEqual() { |
There was a problem hiding this comment.
It may be better to put this also in testIsAlmostEqual() since they are both testing the same function (i.e., isAlmostEqual).
* Rename `elementsApproximatelyEqual` to `elementsAlmostEqual` * Use `elementsAlmostEqual` in implementation of `isAlmostEqual` * Add tests for nan and infinity.
| to other: Tensor, | ||
| tolerance: Scalar = Scalar.ulpOfOne.squareRoot() | ||
| ) -> Bool { | ||
| return self.shape == other.shape && |
There was a problem hiding this comment.
I'm wondering if we want to have this check for mismatching shapes here. @rxwei wouldn't a precondition be better? I believe it would be misleading for this to return false when the shapes are mismatching as it should be expected that it operates over tensors with matching shapes.
Another issue is that of broadcasting. If we want to support broadcasting for this function we should remove that check altogether.
No description provided.