This repository has been archived by the owner. It is now read-only.

Equality semantics for Writable types that are a Number #410

Merged
merged 6 commits into from Sep 14, 2017

Conversation

Projects
None yet
2 participants
@huitseeker
Copy link
Contributor

huitseeker commented Aug 29, 2017

  • ensures hashCode and equals reflects the semantics of the value for (Byte|Int|Long|Double|Float)Writable
  • adds a facade to fuzzyEquals (Guava) for those types

Fixes #265

huitseeker added some commits Aug 29, 2017

@huitseeker huitseeker requested review from AlexDBlack and eraly Aug 29, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Overall idea is good - just some cleanup and tweaks.
Maybe do some tests for edge cases too (like Float infinity should not equal double value larger than largest float).

if (o instanceof DoubleWritable){
DoubleWritable other = (DoubleWritable) o;
float otherFloat = (float) other.get();
return (other.get() == otherFloat && this.value == otherFloat);

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 30, 2017

Member

I'm wondering here: should we be casting up to double for both?
Consider comparing an infinite float with a double larger than the largest float... They will be equal according to this?

This comment has been minimized.

@huitseeker

huitseeker Aug 30, 2017

Contributor

I've added the case explicitly: they won't be equal. This downcasts the Double and returns true if and only if the downcast is w.l.o.g and to the comparison's float. I think it matches the instincts users will expect from how comparisons work in Java. If they want a more permissive match, fuzzyEquals is what should be used.

}

public int hashCode() {
return Float.floatToIntBits(value);
// defer to Float.hashCode, which does what we mean for it to do
return Float.valueOf(value).hashCode();

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 30, 2017

Member

Also cast up here - if a FloatWritable and DoubleWritable are equal, they need to use the same hashcode method.
And as per LongWritable comment - use (copy of) Java 8's Double.hashCode()

This comment has been minimized.

@huitseeker

huitseeker Aug 30, 2017

Contributor

Agreed on the boxing. As to float / double hashCode, the deference to Java makes this work if the Float/Double are equal-izable.

} else if (o instanceof FloatWritable) {
other = ((FloatWritable) o).toDouble();
} else { return false; }
return DoubleMath.fuzzyEquals(this.value, other, tolerance);

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 30, 2017

Member

General concern here: we have this same "instanceof" code block in all the writables... can we pull it out to a single util method?

This comment has been minimized.

@huitseeker

huitseeker Aug 30, 2017

Contributor

I would need this method to return a double even when the writable is not of a numeric type. Short of a crufty encoding (with NaN), properly writing this method will require unnecessary boxing.

On a secondary point, I find the case disjunction makes the cases we accept blatant.

return false;
IntWritable other = (IntWritable) o;
return this.value == other.value;
}
}

public int hashCode() {

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 30, 2017

Member

If Int/Byte/LongWritable values can be equal, they should all use the same hashcode method (i.e., Long hashcode)

This comment has been minimized.

@huitseeker

huitseeker Aug 30, 2017

Contributor

The original hashcodes of these in Java have this property already, and since we're reusing them, they will transfer to writable.

}

public int hashCode() {
return (int) value;
// Defer to the long hashCode, which collides with integer hashCodes on relevant values
return Long.valueOf(value).hashCode();

This comment has been minimized.

@AlexDBlack

AlexDBlack Aug 30, 2017

Member

Let's implement a version of Java 8's Long.hashCode method as a util method - no point producing extra garbage because we don't have a 1-line method...

This comment has been minimized.

@huitseeker

huitseeker Aug 30, 2017

Contributor

Yes, of course.

huitseeker added some commits Aug 30, 2017

@huitseeker huitseeker requested a review from AlexDBlack Aug 30, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

LGTM

@huitseeker huitseeker merged commit 00c68f4 into deeplearning4j:master Sep 14, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.