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

avoid passing GEODESIC_ORDER as argument #10

Open
stonylohr opened this issue Jul 17, 2020 · 2 comments
Open

avoid passing GEODESIC_ORDER as argument #10

stonylohr opened this issue Jul 17, 2020 · 2 comments

Comments

@stonylohr
Copy link
Contributor

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
Copy link
Member

michaelkirk commented Jul 18, 2020

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants