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

Matrix bug fixes and API test #86

Merged
merged 4 commits into from
Jan 30, 2018
Merged

Matrix bug fixes and API test #86

merged 4 commits into from
Jan 30, 2018

Conversation

HendrikLeuschner
Copy link
Collaborator

fixes issues of #63 and adds an api test for matrix


_tmpWeight = _edgeWeight + _entryWeight;

if (_msptSubItem.weight > _tmpWeight || _msptSubItem.weight == 0.0) {
if (_msptSubItem.weight > _tmpWeight || _msptSubItem.weight == Double.POSITIVE_INFINITY) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think just _msptSubItem.weight > _tmpWeight is necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True!

@@ -275,19 +290,18 @@ private void fillEdgesUpward(MultiTreeSPEntry currEdge, PriorityQueue<MultiTreeS
_msptItem = currEdge.getItem(i);
_entryWeight = _msptItem.weight;

if (_entryWeight == 0.0)
if (_entryWeight == Double.POSITIVE_INFINITY)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use Double.isInfinity instead

Copy link
Collaborator Author

@HendrikLeuschner HendrikLeuschner Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the implementation of Double.isInfinite, it its

        return var0 == 1.0D / 0.0 || var0 == -1.0D / 0.0;
 }

of which 50% is the definition of Double.POSITIVE_INFINITY:

public static final double POSITIVE_INFINITY = 1.0D / 0.0;

As I do not want to accept Double.NEGATIVE_INFINITY, which would hint at an error in my code, I am happy to use my version, which should be faster as well.

Copy link
Contributor

@rabidllama rabidllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code seems OK, but we need to make sure that any new methods created which are complicated (especially tests) are well documented.

}

@Test
public void distanceTest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we write a bit of documentation about what the test is for?

double matrixDistance = jDistances.getJSONArray(i).getDouble(j);
Assert.assertTrue( matrixDistance - .1 < routeDistance);
Assert.assertTrue( matrixDistance + .1 > routeDistance);
// .assertThat()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should try and make sure that we don't leave commented out code when we push to dev/master

@@ -21,6 +21,7 @@
package heigit.ors.services.matrix;

import static io.restassured.RestAssured.*;
import static org.hamcrest.Matchers.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do any objects from this package get used? If not then it shouldn't be imported.

@rabidllama rabidllama merged commit f6fc018 into development Jan 30, 2018
@rabidllama rabidllama deleted the Matrix_fixes branch January 30, 2018 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants