Incorrect equality determination

[GregJohnStewart]

With the following code:

Unit<AmountOfSubstance> unit = Units.MOLE;
		
Quantity<?> result = Quantities.getQuantity(0.0, unit).add(Quantities.getQuantity(1.1, Units.MOLE));
		
assertEquals(
	Quantities.getQuantity(1.1, unit),
	result
);

which results in the following error:

AssertionFailedError: expected: tech.units.indriya.quantity.NumberQuantity@30f99ca1<1.1 mol> but was: tech.units.indriya.quantity.NumberQuantity@edb8f0c<1.1 mol>

Any thoughts?

Additionally,

Unit<AmountOfSubstance> unit = Units.MOLE;
		
assertEquals(
	Quantities.getQuantity(1.1, unit),
	Quantities.getQuantity(1.1, unit)
);

		Unit<AmountOfSubstance> unit = Units.MOLE;
		
		Quantity<?> result = Quantities.getQuantity(0, unit).add(Quantities.getQuantity(1, Units.MOLE));
		
		assertEquals(
			Quantities.getQuantity(1, unit),
			result
		);

is fine...

[keilw]

Thanks for bringing that to our attention.
In the first case debugging a slightly simplified example code:

Quantity<?> result1 = Quantities.getQuantity(0.0, Units.MOLE).add(Quantities.getQuantity(1.1, Units.MOLE));
Quantity<?> comparative1 = Quantities.getQuantity(1.1, Units.MOLE);
System.out.println(result1.equals(comparative1));

Shows that result1.getValue() ends up as BigDecimal while comparative1.getValue() remains a Double, hence equality comparison fails here.
The second version involves all Integer values, causing equality to pass.

@andi-huber Any idea how this could be improved? IMO casting to BigDecimal here sounds a bit much, or are we losing precision?
In that case maybe equals() of NumberQuantity could be improved instead, e.g. analyzing the value and compare doubleValue() if one comparative has a type like Double.

[GregJohnStewart]

Agreed, the root seems to be the handling of decimal values and don't have issues with integers.

I have another issue where adding quantities with decimals ends up with less than one of the numbers being added, but haven't been able to replicate simply for an example and might be a separate issue

[andi-huber]

Checking number equality can only be answered within margins of numerical errors, hence I don't recommend using Java equals semantics for this. That may or may not work. Also be aware that even a check for Unit equality is not guaranteed to be decidable. There are corner cases, where our current implementation fails at this: https://github.com/unitsofmeasurement/indriya/wiki/Unit-Equivalence

[andi-huber]

Eg. see how to properly check number equality with JUnit ... https://junit.org/junit5/docs/5.0.1/api/org/junit/jupiter/api/Assertions.html#assertEquals-double-double-double-. (Note the delta parameter, which specifies the margin of error.)

[andi-huber]

As for improvements for Indriya in that regard, (as I believe I said before) we could honor such a delta parameter for Quantity comparisons by providing specific: lessThan, lessThanOrEqualTo, greaterThan, greaterThanOrEqualTo and equalTo methods that support a margin of numerical error parameter.

[andi-huber]

Without change of API, this could also be done with a utility class, that provides some static methods for this.

[andi-huber]

If someone wants to jump in and work on such a utility, I'd be happy to review pull requests.

[keilw]

It seems the bug lies mainly in l 433-436 of DefaultNumberSystem:

if(doubleValue % 1 == 0) {
// double represents an integer
       return narrow(BigDecimal.valueOf(doubleValue));
}

For a doubleValue of 0.0 as in the example this creates a BigDecimal, for 1.1 it doesn't.

@andi-huber Any chance to handle Zero differently or is there a bigger problem with that class?

[andi-huber]

what bug?

[keilw]

Creating an unnecessary BigDecimal from a Double, which is the reason why the equality fails.
While the modulo operation in a way seems correct, 0.0d is still a double and "narrowing" that to a BigDecimal sounds wrong.

[andi-huber]

Well, I do not agree: If the double is an integer number its converted to a type that can safely hold the range a double may represent, which is the BigDecimal in our case. In the next phase(s) we narrow down from BigDecimal to int if possible, BigInteger otherwise.

[keilw]

At least 0.0 is special because it has no value regardless of being integer, double or BigDecimal, it's a bit like null. Treating Zero like any other integer causes this problem (I would not call it a bug IMO because it's the JDK's own comparison between BigDecimal, Integer, Double or other types that fails equality)

The mentioned "utility class" won't help if the value types are different, at most we could try to disect it and use e.g. the doubleValue() part of a BigDecimal if the other value was a Double, intValue() for an int/Integer, etc.

[GregJohnStewart]

Would it make sense to normalize the internal implementation to always use BigDecimal? Would cover any need of operations without having this complexity.

[keilw]

I'm not sure, it would blow up memory consumption and likely slow everything down a bit.
From what I see Seshat even goes the other way by using double only for everything, @desruisseaux did I see that that correct?

The special case of 0.0 failing the modulo check is easy to catch and it'll solve this particular problem.
DefaultNumberSystem even got methods like isZero() (most of those look static btw) to use in this case.

[keilw]

I added a Zero-check to the latest snappshot, but apparently, the "Lambda Magic" in L101 of ScaleHelper turns two Double numbers of 0.0 and 1.1 into a BigDecimal. but it doesn't do that for Integer values.
@andi-huber any idea why this is done there?

[keilw]

Actually it comes back to DefaultNumberSystem where additions always result in a BigDecimal.
It won't solve all problems, but I would handle "non-additions" if either x or y are zero by simply returning the non-zero part as is.

Regarding the suggestion by @GregJohnStewart to handle everything as BigDecimal (ultimately this also had to apply to all integer types, otherwise it does not really make sense) it could be done by adding another NumberSystem (something like BigNumberSystem ;-) similar to what #381 would involve to support Apache Commons Numbers.

[keilw]

The problem was fixed in the SNAPSHOT, @GregJohnStewart could you try it out?

[GregJohnStewart]

To be clear, are you asking I pull the project down and publish the latest locally, or? (I don't see a snapshot on Maven)

[keilw]

It may be good to refresh the local SNAPSHOT but you should not need to build it yourself, because indriya 285 deployed the latest snapshot on the Sonatype Snapshot repo.

The "basic" demos use the 2.1.4-SNAPSHOT and QuantityDemo shows the two are equal now with the latest snapshot. If you try to switch back to e.g. 2.1.3 they are not.

[keilw]

Since there were no contesting statements, I assume this is fixed.

[GregJohnStewart]

First off, apologies for how long it took me to circle back to this.

With the 2.1.4-SNAPSHOT, the problem seems worse; some of the working (in 2.1.3) tests I had already are failing, all to the familiar when-different-types-of-numbers thing, though I will say looks like the original issue in this thread is resolved.

For example:

expected: <1.0> but was: <7.680598941548122E-12>
Expected :1.0
Actual   :7.680598941548122E-12

(Was appropriately 1.0 in 2.1.3)

Looks like a fix here broke other places

[GregJohnStewart]

I'll note that a Quantity(0, unit) != Quantity(0.0, unit) in my tests with the snapshot, where they were working on 2.1.3.

Another of note:

expected: <0.0> but was: <0E-58>
Expected :0.0
Actual   :0E-58

It still appears these issues are strictly dealing with floating point arithmetic, and integer operations are clear.

Let me know if I can provide anything else, any context, etc. If a working/storming session session would help, I'm open to it

[keilw]

This looks weird, but how can one reproduce those?

PRs like #388 might be somehow related, but have to investigate based on test code to reproduce.

[GregJohnStewart]

Quantity<?> result = Quantities.getQuantity(0.0, unit);//.add(Quantities.getQuantity(0.0, Units.MOLE));
		
assertEquals(
	Quantities.getQuantity(0, unit),
	result
);

Results:

expected: <0 mol> but was: <0.0 mol>
Expected :0 mol
Actual   :0.0 mol

Though will note that apparently this probably follows a convention of BigDecimal, were 0 != 0.0, so might not need to fix. At any rate this is easy to account for.

---

Loos like the 7.680598941548122E-12 issue was me doing something silly with Units, see below for an example of the same behavior.

It appears that numbers are thrown off when using derived units (UnitTools.getUnitWithNameSymbol() just adds the unit name, symbol to the unit):

Unit WATT_HOURS = UnitTools.getUnitWithNameSymbol(
	Units.WATT.multiply(Units.HOUR),
	"Watt-Hour",
	"Wh"
);
		
Unit unit = UnitTools.getUnitWithNameSymbol(
	WATT_HOURS.divide(1_000),
	"Milliwatt-Hour",
	"mWh"
);

assertEquals(
	Quantities.getQuantity(0.0, unit).add(Quantities.getQuantity(1.1, unit)),
	Quantities.getQuantity(1.1, unit)
);

Results:

expected: <1.1 mWh> but was: <1.100000000000000000000000000000000088 mWh>
Expected :1.1 mWh
Actual   :1.100000000000000000000000000000000088 mWh

---

assertEquals(
	Quantities.getQuantity(2.3, unit),
	Quantities.getQuantity(1.1, unit).add(Quantities.getQuantity(1.2, unit))
);

expected: tech.units.indriya.quantity.NumberQuantity@115ca7de<2.3 units> but was: tech.units.indriya.quantity.NumberQuantity@29fe4840<2.3 units>

[keilw]

Glad that big difference could be clarified.
I noticed some internal Indriya JUnit tests fail now building it with Java 17. Whether that has something to do with the JVM version or PRs including the one mentioned above I have to check what caused it.

I added a simplified code (without the UnitTools but otherwise quite similar) into QuantityDemo and using Indriya 2.1.3 the only difference is, the first comparison

Quantity<?> result1 = Quantities.getQuantity(0.0, Units.MOLE).add(Quantities.getQuantity(1.1, Units.MOLE));
Quantity<?> comparative1 = Quantities.getQuantity(1.1, Units.MOLE);

remains false while the 2.1.4-SNAPSHOT considers them as equal. So it works for a BaseUnit but not for a ProductUnit like WATT_HOURS.

And yes, the PR #388 caused some serious problems, @andi-huber did you test it locally before merging or after???
Because build 285 on the CI server 2 months ago had all non-skipped tests pass, while 286 after the PR started 17 failures and 1 error. I'll remove that buggy PR and also try to tweak the POM so the build immediately fails in such case.

[keilw]

Reopening this, because it works now (after 2.1.4) for base units, but still fails for most others despite an identical numeric type like Double.
So while

Quantity q1 = Quantities.getQuantity(0.0, Units.MOLE).add(Quantities.getQuantity(1.1, Units.MOLE));
Quantity q2 = Quantities.getQuantity(1.1, Units.MOLE);
System.out.println(q1.equals(q2));

works, returning true.

final Unit<Energy> UNIT_WATT_HOUR = Units.WATT.multiply(Units.HOUR).asType(Energy.class);
Quantity q3 = Quantities.getQuantity(0.0, UNIT_WATT_HOUR).add(Quantities.getQuantity(1.1, UNIT_WATT_HOUR));
Quantity q4 = Quantities.getQuantity(1.1, UNIT_WATT_HOUR);
System.out.println(q3.equals(q4));

returns false.

While it seems to work properly for base units, the method ScaleHelper.addition() does something weird only intended for a RELATIVE scale despite an ABSOLUTE one for a ProductUnit.

@andi-huber Any ideas?

[keilw]

Seems

converting almost all, except system units and those that are shifted and relative like eg. Δ2°C == Δ2K

Is the problem. It converts everything to system units except base units, therefore even cases where both units are identical face an unnecessary conversion of the unit. And transformation of the number.

[keilw]

I tried to bypass this for identical units, but frankly I'm out of options here because it breaks precisely those special cases like
TemperatureTest.addingAbsoluteTemperatures() or
QuantityFunctionsTemperatureTest.testSumTemperatureC().
Therefore ScaleHelper cannot be changed.

Any unit other than BaseUnit results in a converted result where the value is a value RationalNumber and therefore different from any other Number type.
The only way this works is, if a test or application code looks like:

final Unit<Energy> UNIT_WATT_HOUR = Units.WATT.multiply(Units.HOUR).asType(Energy.class);
Quantity<?> q5 = Quantities.getQuantity(BigDecimal.ZERO, UNIT_WATT_HOUR).add(Quantities.getQuantity(BigDecimal.valueOf(1.1), UNIT_WATT_HOUR));
Quantity<?> q6 = Quantities.getQuantity(RationalNumber.of(1.1), UNIT_WATT_HOUR);
System.out.println(q5.equals(q6));

This is true.