Move geodesy code to an independent package

[garborg]

In splitting out code into newly-released Geodesy.jl, a few changes were made:

• Point types made immutable => cheaper construction, access for Dict{Int, PointType}
• More accurate construction of datums => slightly more accurate conversions
• Faster conversion
• Fixed a bug in boundaryPoint that made it return the interior point half the time
• Made conversion more (but not yet fully, see garborg/Geodesy.jl#4) idiomatic
• Ability to specify alternate datum for all conversion methods (added a couple datums, too)
• Handle Bounds that cross the antimeridian
• More accurate Bounds conversion
• Expanded testing

[coveralls]

Coverage increased (+3.05%) when pulling a1f33685b0783eef02af1fea6527490d4335ce56 on geodesy into e2b26812506fe0845f1cd0ffcb58df63cd686bb9 on master.

[garborg]

Same old Travis-only LibXML segfault on 0.4 -- time to prioritize #60, I suppose.

[yeesian]

Looks great, thanks for this!

Is there a reason why the remaining functions left in src/transforms.jl are not moved to Geodesy.jl as well?

[garborg]

Dict{Int,ENU} just seemed like a pretty OpenStreetMap.jl-specific method to go into Geodesy.jl before, say, Vector{ENU}. The whole thing would probably go away in favor of a generic map implementation if inlining function arguments happens in Base. How about we wait until there's a second, non-OSM package that wants it?

[yeesian]

Ah okay, I was just curious, since there's no dependency on types defined in OSM.

[garborg]

Oh yeah, that's a good point, too.

[garborg]

On the other hand, I'm guessing Dict{Int,ENU} may get a Blah{ENU} alias in OpenStreetMap.jl before too long, and that the method will move from ENU(dict) to more idiomatic Blah{ENU}(blah).

[yeesian]

That might be true, yeah. I personally prefer sticking to base julia types where possible though (hence the removal of cruft like KeyVertex in https://github.com/tedsteiner/OpenStreetMap.jl/pull/58), and to think of OSM as providing functions for parsing osm files transparently into julian types, as well as convenience functions (which might get refactored out into other packages later) -- for plotting, analysis, transformations/projections/etc.

Imagine if Blah{ENU}(blah) gets introduced later in OSM, and we provide the tests etc for it. Some months later someone else comes along, finds Geodesy.jl useful (but lacking) for their work, asks if anyone else has done the work of src/transforms.jl before, gets no reply (because we happen to be afk), and so rolls up their sleeves and opens a PR for it etc -- and we return to find the duplication of effort/logic/code in both OSM and Geodesy.jl. I would have dismissed it as improbable, if it didn't happen in Graphs.jl some weeks back.

On the other hand, it doesn't seem like too much effort on our part to provide say.. Dict{Int,ENU} and Vector{ENU} in Geodesy.jl from the get-go. If (and when) we refactor the code in OSM to Blah{ENU}(blah), it will be just a local change to OSM to re-use the function (defined and tested in Geodesy.jl). This is what we're trying to work towards with src/routing and Graphs.jl too.

[garborg]

Definitely not too much effort, but given that the both ENU(Dict{Int,LLA}, ..) and ENU(Vector{LLA}, ...) aren't meant to exist in the long-term (they're workarounds for current Julia limitations), and if they were to stick around they'd need to change from ENU(dict, ...) to Dict{Int,ENU}(dict, ...) or Nodes{ENU}(nodes, ...) (a name like Nodes being nicer for this package than any generalized name I can think of at the moment, but definitely not belonging in Geodesy.jl), I'd kind of like to avoid introducing that much churn across packages. I'll point garborg/Geodesy.jl#4 back here explicitly with a summary.

[yeesian]

Ah that's fair. I just saw your comment about "making conversion more (but not yet fully, see garborg/Geodesy.jl#4) idiomatic", and agree about it being OSM-specific the longer and closer I stare at the functions in question (across src/routing and src/transforms.jl)

[yeesian]

LGTM. @tedsteiner?

[tedsteiner]

Everything looks good to me, too! At one time I had thought about moving the transforms myself into something like Geodesy.jl (see #9), but I kept putting it off because I wasn't sure what the best way to abstract the types and conversions would be with Julia syntax. So, thank you, @garborg, I think it looks great! I'm also happy to hear that this code will be more widely useful for people, and thank you for including a reference back to this package.

I will defer to you both on the actual design of the types, but what you have looks good to me. I do have a couple questions, though:

1. What does it mean that the types of immutable? Does this mean that a node cannot have it's location (values) changed?
2. I see that you changed the names of some of the fields to look pretty (e^2, for example). I realize those fields are pretty rarely accessed externally, but is it easy to type those characters into a text editor like Vim or Gedit? I'm wondering if it will cause a hassle for anyone who does need access to them.
3. Something I meant to do but apparently never got around to was make all of the coordinate types inherit from an abstract type, such as Point.

Personally, I think the conversions of the node dictionaries is fine to go in either place. I think it makes for a convenient map representation, so perhaps others will realize this as well and do something similar, even if not working with OSM data. While it may be slower than a vector of locations, its certainly more robust if you will be manipulating the map. I definitely see @yeesian's point about us not being able to monitor everything that's going on and avoid duplication of work.

I don't have any planned updates to this package for a bit, so I'm fine with us completing the merge, as long as all the testing errors are related to #60.

[garborg]

1.) Immutable here means that you can't reach inside an instance of the type and change one of its fields (intuitive here, because unlike with type Boat; name; bearing; position::LLA; end where you might want to say the bearing and position could change over time and represent the same boat, if latitude of the point changed, it would not represent the same point)

• You can still change the point a node id maps to, by assigning a new point to the same id -- just not by mutating the fields of the existing point.
• The downside would be that that you couldn't create a function that used external data to update a set of points' altitudes in place or something like that (the function would create a new set of points).
• The upside is that existing code will perform a bit better, and the code is a little easier to reason about.

2.) Honestly I just copy and pasted characters in from the repl, because it didn't feel significant enough to install a Latex autocompletion plugin (it is built into the Julia repl, though, type \prime + [tab] or \theta + [tab] or \^2 + [tab], etc.). People have different preferences, and I'm kind of in the middle -- I think it's okay for mathy functions (with so many less characters, and matching the text books, I find those conversion functions easier to audit and tune now) where the no user is meant to ever type them (the raw constructor that has non-ascii fields doesn't take keyword arguments), but I agree that it could be mildly annoying to a package contributor.

3.) Inheriting makes sense. I wasn't sure how to do it yet (will distance someday support LLA and will Bounds someday support ECEF, or would there always need to be two abstract types under the main abstract type?, etc.) and then I forgot -- is good to have it on the radar as a TODO.

The testing error is the same XML Travis-0.4-only error that we've failed to reproduce locally before (everything works locally for me on 0.4 before and after the PR) -- given that it fixes a real bug (in my boundaryPoint code =/), I'd feel good about merging.

Didn't mention it before, but reexport means after using OpenStreetMap, all names exported by Geodesy are available in your namespace -- half of it was exported before this PR, but exporting the second half has been nice for user code, imo.

[tedsteiner]

That reasoning all makes sense to me, thanks for the explanations. I agree with all your arguments. For the exports, it doesn't really make much difference for me, since I very rarely use using in my code, since I always want to know where things are coming from in an obsessive kind of way.

If you feel good about merging, I feel good about it. I vote go for it.

[garborg]

Oh yes, that's a good practice, and I should do that more (just to clarify, the thing that felt confusing to me before was getting unqualified access to the methods that operate only on points and bounds types without being able to refer to those types without qualifying them).

Okay, feeling good about it -- merging!