Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tec.units.indriya.quantity.QuantityDimension equals() bug #207

Closed
solr00t opened this issue Apr 4, 2019 · 12 comments
Closed

tec.units.indriya.quantity.QuantityDimension equals() bug #207

solr00t opened this issue Apr 4, 2019 · 12 comments
Labels

Comments

@solr00t
Copy link

solr00t commented Apr 4, 2019

I wrote some fuel efficiency units, and discovered a bug in dimension comparison:

public interface FuelConsumption extends Quantity<FuelConsumption> {
	@SuppressWarnings("unchecked")
	public static final Unit<FuelConsumption> L_PER_100KM
			= (Unit<FuelConsumption>) Units.LITRE.divide(Units.METRE).divide(100000);
}
public interface FuelEconomy extends Quantity<FuelEconomy> {
	@SuppressWarnings("unchecked")
	public static final Unit<FuelEconomy> MILES_PER_GALLON
			= (Unit<FuelEconomy>) USCustomary.MILE.divide(USCustomary.GALLON_LIQUID);
}
public class Test {
	@SuppressWarnings("unchecked")
	public static final Unit<FuelConsumption> L_PER_100KM
			= (Unit<FuelConsumption>) Units.LITRE.divide(Units.METRE).divide(100000);
	
	public static void main(String[] args) {
		Dimension fcDim = FuelConsumption.L_PER_100KM.getDimension();
		//Dimension fcDim = Test.L_PER_100KM.getDimension();
		System.out.println("FC dim: " +  fcDim);
		
		Dimension feDim = FuelEconomy.MILES_PER_GALLON.getDimension();
		System.out.println("FE dim: " +  feDim);
		
		Dimension feInvDim = feDim.pow(-1);
		System.out.println("FE inv dim: " +  feInvDim);

		System.out.println("fcdim == feInv ? " +  fcDim.equals(feInvDim));
	}

}

Result:
FC dim: [L]²
FE dim: 1/[L]²
FE inv dim: [L]²
fcdim == feInv ? false

And very strangely, if you switch out the comments on the fcDim variable, here is the output:
FC dim: [L]²
FE dim: 1/[L]²
FE inv dim: [L]²
fcdim == feInv ? true

Why am I getting a different result depending on where I declare the unit? Can anybody else replicate this?

@keilw keilw added the analysis label Apr 4, 2019
@keilw
Copy link
Member

keilw commented Apr 4, 2019

Thanks for your observation. It seems at least related to #33 and there is also a ticket in the API project: unitsofmeasurement/unit-api#89.

@andi-huber
Copy link
Member

Its not clear to me, how to reproduce the issue.
However, I've added a TestCase [1] to exercise above use-case.

[1] https://github.com/unitsofmeasurement/indriya/blob/master/src/test/java/tech/units/indriya/quantity/QuantityDimensionEqualityTest.java

@andi-huber
Copy link
Member

andi-huber commented Apr 5, 2019

Code Review:

Javadoc from javax.measure.Dimension

Two units u1 and u2 are compatible if and only if u1.getDimension().equals(u2.getDimension()).

So why is RI deviating from this? According to the spec, this predicate should be returned unconditionally, right?

if (thisDimension.equals(thatDimension))

@andi-huber
Copy link
Member

Suggested improvement: re-implement QuantityDimension.equals such that AbstractUnit.internalIsCompatible can return thisDimension.equals(thatDimension) ...

  private final boolean internalIsCompatible(Unit<?> that, boolean checkEquals) {
    if (checkEquals) {
      if (this == that || this.equals(that))
        return true;
    } else {
      if (this == that)
        return true;
    }
    if (!(that instanceof AbstractUnit))
      return false;
    Dimension thisDimension = this.getDimension();
    Dimension thatDimension = that.getDimension();
    return thisDimension.equals(thatDimension);

//    if (thisDimension.equals(thatDimension))
//      return true;
//    DimensionalModel model = DimensionalModel.current(); // Use
//    return model.getFundamentalDimension(thisDimension).equals(model.getFundamentalDimension(thatDimension));
  }

@solr00t
Copy link
Author

solr00t commented Apr 5, 2019

Its not clear to me, how to reproduce the issue.

By this, do you mean that when you run the code that I pasted (as three standalone classes) that you get different results? Or do you mean that it's hard to replicate the problem in a single standalone test case? I'm curious if this is something particular about my environment.

@andi-huber
Copy link
Member

I failed to reproduce the issue trying:

  • single standalone test case (as currently provided)
  • moving the FuelConsumption interface into its own file

However, I'd love to know what's causing this.

@andi-huber
Copy link
Member

Also I'm curious, whether you do have the same issue, if you try a different method (for the dimension equality check) as shown in the second test in [1].

[1] https://github.com/unitsofmeasurement/indriya/blob/master/src/test/java/tech/units/indriya/quantity/QuantityDimensionEqualityTest.java

@solr00t
Copy link
Author

solr00t commented Apr 5, 2019

When I run your test case, both dimensionsShouldBeEqual() and unitsShouldBeCompatible() are failing. For some reason, FuelEconomy.MILES_PER_GALLON.getDimension() is returning a dimension of [L]/null. Are these test cases completing successfully for you?

@andi-huber
Copy link
Member

Surefire Tests seem to fail as well. I guess, bumping JUnit versions earlier this day was a bad idea on my end. I'm reverting this commit ...

@andi-huber
Copy link
Member

... after having reverted my JUnit 'version bump' Surefire Tests do succeed again.

@solr00t
Copy link
Author

solr00t commented Apr 5, 2019

I think the issue might be that I'm working from the current maven version 2.0-EDR (should have mentioned this in my first post), while I'm guessing you're working from the current snapshot which has probably fixed a few of the problems I'm finding. Are there any snapshots on Maven, or do I need to just clone all the GitHub repositories into my workspace?

@solr00t
Copy link
Author

solr00t commented Apr 5, 2019

I found out how to get the snapshots by pointing to https://oss.jfrog.org/artifactory/oss-snapshot-local. This bug has been fixed in the current snapshot. I am now passing the test case that you posted. Thanks!

@solr00t solr00t closed this as completed Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants