Perf/span tweaks #44
Merged
Perf/span tweaks #44
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* make adjustments to MapProjection classes to benefit from new Transform overload (e.g. RadiansToMeters, MetersToRadians, ...) * remove unnecessary overloads of other Transform methods * Add Converter class that helps with ICoordinateSequence and Span<double> #32
Also remove junk that was only needed when we were multi-targeting.
* Tweak how we use spans.
- Base implementations now have virtual methods that can be overridden as appropriate to provide optimized versions for any of the following layouts:
- [x0, y0, z0, x1, y1, z1, ..., xn, yn, zn]
- [x0, y0, x1, y1, ..., xn, yn, z0, z1, ..., zn]
- [x0, x1, ..., xn, y0, y1, ..., yn, z0, z1, ..., zn]
All built-in algorithms now only override the one-by-one variants. The point of allowing subclasses to override for spans (and allowing different layouts) is to allow implementations that use SIMD and/or auxiliary DLLs for their implementations. There ought not to be a **particularly** significant advantage to using spans the way we were doing before.
* Bleh, this was supposed to use the arrays, not the spans, to be a tiny bit cheaper on the crusty ol' .NET Framework with basically no extra development effort
* Improve performance unit test * add various wkt files for transformation * Transformer/Converter approach interchangeable with SequenceCoordinateConverter compile time constant.
* Adjusted PerformanceTests to play along with BenchmarkDotNet
- ProjNet4GeoAPI code is now a much larger fraction of the benchmark - Added a thing at the start to validate that everything's actually correct - Parameterized it instead of using compiler flags so that we can see it all in just one report - Optimized some implementations a little bit more
* Removed (redundant) bulk transform overloads * Moved XY and XYZ structs to ProjNet/Geometries * Added lots of documentation * Seperated some projections into own files
airbreather
reviewed
Apr 27, 2019
There was a problem hiding this comment.
Here's what I've got... I left out a lot of unimportant things that would make this take forever and are way easier to just fix instead of logging them.
There's a lot, so I'm fine with making any or all of these changes myself after it's merged to develop (since what I assume is the point of having develop in the first place), so I'm perfectly happy if you want to merge with stuff left unresolved, as long as you indicate the things that I shouldn't touch.
ProjNet.Tests/Geometries/Implementation/SpanCooordinateSequence.cs
Outdated
Show resolved
Hide resolved
ProjNet.Tests/Geometries/Implementation/SpanCooordinateSequence.cs
Outdated
Show resolved
Hide resolved
ProjNet.Tests/Geometries/Implementation/SpanCooordinateSequence.cs
Outdated
Show resolved
Hide resolved
ProjNet.Tests/Geometries/Implementation/SpanCooordinateSequence.cs
Outdated
Show resolved
Hide resolved
ProjNet.Tests/Geometries/Implementation/SpanCooordinateSequence.cs
Outdated
Show resolved
Hide resolved
| /// <returns>Converted point.</returns> | ||
| protected (double lon, double lat, double z) MetersToDegrees(double x, double y, double z) | ||
| { | ||
| (double lon, double lat, double _) = MetersToRadians(x, y, z); |
There was a problem hiding this comment.
why discard resulting Z?
shouldn't it be more like:
double lon, lat;
(lon, lat, z) = MetersToRadians(x, y, z);There was a problem hiding this comment.
Because it is not touched. I have removed z from MetersToRadians and RadiansToMeters signature altogether.
that take Span<T> arguments to work in-place. * Changed MapProjection to only work on lon/lat or x/y. z was never touched. * Updated SequenceCoordinateConverters to respect new core transform method
airbreather
reviewed
May 2, 2019
| if (elementsX != elementsY) | ||
| throw new ArgumentException("Spans of ordinate values don't match in size."); | ||
|
|
||
| if (zs == default) |
There was a problem hiding this comment.
I would rather check zs.IsEmpty here since I'm pretty sure you can have zero-length spans pointing to non-null data.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes on @airbreather work according to #43.