georust / geographiclib-rs

A port of geographiclib in Rust.
MIT License
41 stars 9 forks source link

avoid passing GEODESIC_ORDER as argument #10

Open stonylohr opened 4 years ago

stonylohr commented 4 years ago

The current code has moved several functions that refer to this constant out of geodesic and into geomath, where the constant is not available. Instead, it is passed as an argument. I think it would be preferable to allow it to be referenced directly as a constant, rather than passing as an argument. This is partly a style choice, but should also yield a slight performance gain.

This could be done by either moving the constant from geodesic to geomath, or by moving the consuming functions from geomath back to geodesic. The choice of which approach to use might depend in part on what motivated the decision to move those functions to geomath in the first place. All other things being equal, it seems preferable to define the functions in geodesic, for symmetry with sibling versions of geographiclib.

michaelkirk commented 4 years ago

This makes sense, though I haven't audited to see if there are places where the Math methods which take a GEODESIC_ORDER always take the GEODESIC_ORDER.

All things being equal, it is of course nice to prefer implementations that resemble the reference implementation. We might consider using macros to get the same perf boost while matching Karney's usage of templates, but maybe it's not worth the hassle and just moving the constant to geomath would be better.

I don't know that there was a particular reason for them being in one file or the other - I'd say matching the reference implementation structure wherever possible is a laudable goal provided it doesn't hurt readability too much.

stonylohr commented 4 years ago

In the original C++ implementation, the functions that use geodesic order refer to the constant directly, rather than taking it as an argument. It's possible that the python port adds features that involve it being variable, in which case passing as an argument would make sense.

From what I've seen, most of Karney's templates in the C++ version seem to be aimed at allowing support for extended-precision floating point types. As far as I can tell, rust doesn't currently have support for these, and it's not clear that it's likely to anytime soon. Until we start seeing indication of higher-precision types on the horizon, I would lean toward supporting only double precision floating point types, which seems to be the approach taken by all the ports I've looked at other than the original C++. Of course, you may be referring to a different set of templates.