Updated to NTS 2.0 and removed use of GeoAPI where required. Also Fixed 4 Bugs and Fixed most of the Unit Tests

[juliusfriedman]

I just made this because since I am having issues with or without contractions I figured I should help to try and track the issue down, this was a low hanging fruit and gets the later version of NTS in the library.

All tests seemed to pass on my machine so it's possible this threading issue is fixed in the develop branch of reminiscence, I will add a few more tests to make sure but there was already GetTestRandomRoutesParallel but it was not being run so I added that to the list of tests to run.

[juliusfriedman]

This should resolve #256

[juliusfriedman]

@xivk , this should be okay to merge now, let me know if you find anything else.

Also please note that the nugets seem to be built for x86 which may have caused some of the other issues I was reporting about OOM

[juliusfriedman]

There was also a reference to the older NTS library from OSM Sharp which I fixed in OsmSharp/core#101

[juliusfriedman]

Sorry for any inconvenience and please let me know if you need anything else to get this merged easily and thank you for your interest / hard work!

[juliusfriedman]

Also addresses #304

[juliusfriedman]

Also wondering a similar fix should be applied @

routing/src/Itinero/Router.cs

Line 1176 in 11fe52e

weights = Itinero.Algorithms.Contracted.Dual.RouterExtensions.CalculateManyToMany(contracted, _db, profileInstance.Profile,

[juliusfriedman]

@xivk, please let me know when you get a chance to review this and the other PR's submitted.

[xivk]

Hey @juliusfriedman I appreciate all this hard work but the pull request has multiple changes on different features and some features of a new C# version are introduced. All these things that are great as improvements but it's gotten a bit more difficult for me to review.

Is this getting merged something you need urgently? I should be able to check this next week Wednesday. Sorry, struggling to keep up with everything these days.

[juliusfriedman]

No problem, I didn't mean to make your life harder.

The fix I would need most importantly relates to reminisce because of the usage of Parallel. The quick fix here would be to lock on the db but it's no longer required after the change in reminisce.

The importance of that change is due to differences seen when routing using parallel or outright exceptions.

Take your time as I also addressed a distance calculation issue and made a change to use the ManyToMany algorthimm to resolve issues with weight handling which was reported by other users.

Next week should be fine. Just let me know if there's anything I can do to make this easier on you (e.g. I didn't see any changes which would require any recent c# features, if you can point them out I'll surely change them up to something which doesn't require a newer compiler)

[juliusfriedman]

@xivk I double and triple checked this and added notes on what was addressed. Please let me know ASAP if this is correct so I can close my PR's and delete my branches.

[juliusfriedman]

@xivk I don't see any usages of any newer C# features or langversion changes to support them, if you are seeing something please point it out to me so I can change it.

Thank you!

[juliusfriedman]

Was removed and the issue no longer manifested in the simple case

[juliusfriedman]

As you can see from the commented code above CalculateManyToMany was previously being used but returned different results which didn't match that of the non contracted versions. This change fixed both the simple and complex cases I was made aware of.

[airbreather]

Just let me know if there's anything I can do to make this easier on you

@juliusfriedman looking through this PR, it looks like you've lumped many different things together. I would recommend making one PR that just does the NTS 2.0 upgrade, since that should be mostly signature changes that happen to affect many files and therefore much easier to review that part of it. Afterward, you can make other changes on top, and each would be easier to review on their own.

For example, consider 2fe7e3d. Looking at that, I question the need for it... ignoring the (fixable) problem that the new test provides latitude values outside of the legal range, is the intent to try to improve accuracy over very large distances?

• If that's the case, then is that worth the incremental performance cost of 4 trig functions and 1 sqrt function per call*, in Itinero? The name already suggests that it's going to be inaccurate, and it's still going to be inaccurate because the Earth isn't a perfect sphere, so I expect speed to matter a lot more than accuracy.
• My point here isn't to specifically resolve this one point of discussion, but rather to show that the more unrelated changes you pack into one PR, the more different things a reviewer needs to think about; one minute I'm looking at a change from GeoAPI --> NetTopologySuite, and the next I'm on the Wikipedia page for great-circle distances.

If you also want to fix some bugs, then I'd recommend doing that in other PRs; if those fixes affect code that's tightly coupled to NTS, then maybe it would make sense to wait to submit those until the NTS 2.0 upgrade is merged, since merge conflicts would add friction to that effort.

*plus multiple Math.Pow calls, though all of them are Math.Pow(x, 2), so we could optimize them by doing x * x ourselves, so it would be unfair to call these out.

[juliusfriedman]

//https://github.com/itinero/routing/issues/279
            const double DegreesToRadians = Math.PI / 180.0;
            var d1 = latitude1 * DegreesToRadians;
            var num1 = longitude1 * DegreesToRadians;
            var d2 = latitude2 * DegreesToRadians;
            var num2 = longitude2 * DegreesToRadians - num1;
            var d3 = Math.Pow(Math.Sin((d2 - d1) / 2.0), 2.0) + Math.Cos(d1) * Math.Cos(d2) * Math.Pow(Math.Sin(num2 / 2.0), 2.0);

            return (float)(6376500.0 * (2.0 * Math.Atan2(Math.Sqrt(d3), Math.Sqrt(1.0 - d3))));

What x * x can replaced with the Math.Pow call here?

Math.Sin(num2 / 2.0) * 2.0;

Perhaps also (more so):

Math.Pow(Math.Sin((d2 - d1) / 2.0), 2.0)

            const double DegreesToRadians = Math.PI / 180.0;
            var d1 = latitude1 * DegreesToRadians;
            var num1 = longitude1 * DegreesToRadians;
            var d2 = latitude2 * DegreesToRadians;
            var num2 = longitude2 * DegreesToRadians - num1;
            var num3 = Math.Sin((d2 - d1) / 2.0);
            num3 *= num3;
            var num4 = Math.Sin(num2 / 2.0);
            num4 *= num4;
            var d3 = num3 + Math.Cos(d1) * Math.Cos(d2) * num4;
            return (float)(6376500.0 * (2.0 * Math.Atan2(Math.Sqrt(d3), Math.Sqrt(1.0 - d3))));

That should be better but AFAIK the JIT recognizes these types patterns and optimizes anyway.

The changes were done like this because they required changes in Remininscence, those have been accepted and thus these can now be reviewed and accepted as well.

Not sure what you mean by breaking them down into parts all parts effects almost all others.

[juliusfriedman]

And just to be clear:

With my fixes and some as of yet uncommitted all tests cases are passing in Itinero.Algorithmns.Default.EdgeBased

Not be to a jerk or anything but this PR sat here for weeks (maybe more) so I do not feel the onus is not on me here to break this down.

If something is not desired simply remove it during the merge.

[airbreather]

What x * x can replaced with the Math.Pow call here?

Again, my point isn't to specifically resolve this one, but to answer this question:

//https://github.com/itinero/routing/issues/279
    const double DegreesToRadians = Math.PI / 180.0;
    var d1 = latitude1 * DegreesToRadians;
    var num1 = longitude1 * DegreesToRadians;
    var d2 = latitude2 * DegreesToRadians;
    var num2 = longitude2 * DegreesToRadians - num1;
    var temp1 = Math.Sin((d2 - d1) / 2.0); // extracted from the expression that produces d3
    var temp2 = Math.Sin(num2 / 2.0); // extracted from the expression that produces d3
    var d3 = (temp1 * temp1) + Math.Cos(d1) * Math.Cos(d2) * (temp2 * temp2);

    return (float)(6376500.0 * (2.0 * Math.Atan2(Math.Sqrt(d3), Math.Sqrt(1.0 - d3))));

Not sure what you mean by breaking them down into parts all parts effects almost all others.

One specific suggestion I made was to start with a PR that only does the NTS 2.0 conversion. This would be easier to review and less likely to break things, and it would make the remainder easier to review as well, since it would all stand out on its own.

With my fixes and some as of yet uncommitted all tests cases are passing in Itinero.Algorithmns.Default.EdgeBased

All that that means is that the tests pass. Every (valid) user-visible bug is further evidence that the automated test suite is either incomplete or incorrect.

For example, I already noted that the new test for #279 is providing latitude values outside of the legal range. This test is therefore incorrect (how can the expected value be any number, when the two inputs don't even refer to actual locations?), yet it passes. For now, code review is our primary defense against those kinds of problems.

this PR sat here for weeks (maybe more) so I do not feel the onus is not on me here to break this down.

I would argue the opposite, actually: it takes longer to review more changes that do more things than to review fewer changes that do fewer things, and reviews tend to be overall more effective when they're focused on just one thing. Of course, if that "one thing" is a solution to a complicated problem that truly requires making many changes that affect one another, then it can't be helped.

From my perspective, the only potential breakdown I specifically commented on was "NTS 2.0 / everything else", for a couple of reasons:

1. I don't know the details of the Itinero architecture well enough, nor did I scan through enough code, to be able to conclusively say how much of "everything else" can be broken down further, and
2. I do know what kinds of changes we made in NTS 2.0, and what kind of impact they should have on downstream solutions. For sure, the need to upgrade the OsmSharp.Geo (and therefore OsmSharp) package references to prereleases of newer major versions complicates this significantly, but changes like the one I happened to notice and comment on above suggest that there are still some opportunities to split out at least one other PR (possibly more).

If something is not desired simply remove it during the merge.

I don't have merge privileges, so it's not my call. I responded because you invited this kind of feedback:

please let me know if you need anything else to get this merged easily

and

Just let me know if there's anything I can do to make this easier on you

Since it's been a couple of months since @xivk last responded on this thread, I felt like you may benefit from hearing this perspective. Maybe I'm wrong, and this PR is exactly what he wants to see, but after reading his last comment on this thread, I would guess otherwise.

[juliusfriedman]

That comment he made I believe he was referring to at that time which was missing pieces which would not compile and contained local references to the Reminiscence project and those conversations have been resolved AFAIK.

The last comment he made was about newer C# features (which are not used anywhere outside of what was already used) so I was confused and applied only the last change for: #306

Which I began to test however I ran into additional problems when resolving as I could no longer contract databases however I can use them after contraction so long as the fix in #306 is applied after the contraction is made.

I have no idea why and I have not found a case where it effects me so I did not move further on that.

Beyond that it seems there are multiple places that Itinero.Algorithms.Contracted.Dual.RouterExtensions.CalculateManyToMany [which uses VertexToVertexWeightAlgorithm only 2 reference to such in the entire project and NO unit tests (See also https://github.com//issues/304) and https://github.com//issues/293] provides incorrect results however when it's replaced with Itinero.Algorithms.Default.ManyToMany those issues are gone. I needed @xivk to advise but I believe it should either be Itinero.Algorithms.Contracted.Dual.RouterExtensions.CalculateManyToMany should either be fixed or removed in favor of Itinero.Algorithms.Default.ManyToMany however I am not sure that Itinero.Algorithms.Default.ManyToMany uses the contraction based optimizations ...

The same seems to occur with ManyToManyWeightsBidirectionalDykstra as well as Dykstra or vice versa not sure, however again when that code instead uses Itinero.Algorithms.Default.ManyToMany the problem resolves.

The last thing I have to say is that the automated tests are failing because there are incorrect tests in the code for example TestBicycleWithCyclenetwork either that or the logic which provides the result is incorrect, my fixes for this are noted above but not in the PR.

Let me know how else I can be of assistance @xivk or @airbreather

P.S. The changes for distance were for correctness regardless of input, a check to ensure they are valid is technically outside the scope of that function because floats are given and not Coordinate instances, and when Coordinate instances are provided their Longitude and Latitude is passed in the correct order to the function so I am not sure what you problem exactly is. Adding validation logic there for -180 < || > 180 || <-90 || >90 is trivial but performed way before that code is executed at least as called from Itinero as they have first been converted to Coordinate by LocalGeo.

Finally even when Coordinates are passed in the result of distance must still be determined from Itinero because of the varying Spatial Systems i.e. Geometry vs Geography and their SRID which may be in use and the complexity involved with converting them, thus the approach given (which we use in a production environment and works well) is very suitable because it does not rely on Spatial System (and neither did the previous); the main problem with the previous was given two close coordinates in opposite hemispheres the distance would be incorrect previously however as it stands in this PR that is not the case.

See also #307

[juliusfriedman]

@airbreather

Everything passes in this PR and I fixed the order of the Coordinate tests.

The only thing which does not pass is RouterTests.TestBicycleWithCyclenetwork and that's because in this code the last 2 nodes are null, not sure if this a typo or intended.

[juliusfriedman]

Finally I also adjusted for your Math.Pow2 calls to X*X but please remember the JIT should optimize Math.Pow these especially in later builds See also dotnet/runtime#31978

[juliusfriedman]

Waiting for another review or merge @xivk, @airbreather .

In my local code I have another few things which are as of yet uncommitted e.g. the use of CalculateManyToMany not sure if replacing that with the algorithm is the best thing to do if there are optimizations that CalculateManyToMany uses that the algorithm does not.

The odd thing is that if these types of issues don't occur using the AugmentedWeightHandler then perhaps when contracted it should become the default?

Just lmk.