-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add gnomonic projection #42
base: main
Are you sure you want to change the base?
Conversation
bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ondt and thank you for the pull request!
Before I dig into the code too deeply, I've noticed the output seems to be pretty light on tests.
I'd prefer to have multiple test cases testing against the exact output of the reference implementation, as we do with the other geodesic stuff.
Do you know how upstream tests this feature?
Let me know if you have questions.
I couldn't find any official test cases or testing procedures for this feature in the upstream repository. What should be the next course of action in this case? Should I create my own test cases or should I search for some generic test cases for projections? |
One approach:
See https://github.com/georust/geographiclib-rs/blob/master/src/geodesic.rs#L2645 for how we've done this with the "direct" and "inverse" geodesic calculations. We have a small test set in the repository: https://github.com/georust/geographiclib-rs/blob/master/test_fixtures/GeodTest-100.dat You can download the larger test set for those calculations by running: You can read more about that input file (including it's format) here: https://github.com/georust/geographiclib-rs/blob/master/src/geodesic.rs#L2575 Ideally generating a similar input for our tests from Karney's output could be relatively scripted so that we could re-run it if Karney changed something. What do you think? |
Thanks for the explanation! Since we might not want to just store a large test set somewhere, I think the following approach might be feasible:
Steps 1..=3 could be performed by some semi-external script that would depend on the Karney's toolkit. |
Thank you for laying out an approach. It sounds basically good, but I'd prefer to simplify it a bit:
Right, as you probably noticed for the existing tests, we store a small test set in repo, and then script/download-test-data.sh downloads a medium length one (confusingly, called "short"), and a larger one. It sounds like you were talking about generating the test input dynamically as part of CI. That's an interesting idea, but would you be ok instead to run it once manually and upload the generated output for starts? Then, like we do with the existing geodesic tests, we'll just download these static inputs. The primary motivation for this request is that it introduces additional complexity, and I'd like to keep your first PR as small as possible and CI us unfiddly as possible for starts. So specifically I'm thinking:
If everything works out, we can revisit generating the test data as part of CI in a followup. The approach could work well for any future geographiclib-rs functionality we port too. We might even consider having a random seed for some kind of fuzz testing against Karney's impl.
I sort of implied it above, but will reiterate: Since CI won't need to run the input generation, I'd prefer to start with the simplest possible script, even if it requires some fiddling to run - e.g. it might assume that Does all that make sense? Is that agreeable? |
BTW the documentation at https://zenodo.org/record/32470 does look like a reasonable foundation - good find! I'm not sure if there are other edge cases that we'd want to emphasize in the test set for gnomonic, but the ones emphasized in TMcoords.dat seems like a reasonable start. |
These exact numbers (10k vs. 500k) aren't that important to me btw, I think you understand that we're looking for a tiny in-repo test set plus a medium and huge one stored externally. |
Thank you for the feedback. The general idea seems good to me. However, I'd rather write the "script" in Rust than as a shell script, so I think creating a very simple library crate (that could pull in |
Provided it's a small number of lines of code, it should be easy to review, so that's fine with me. |
CHANGES.md
if knowledge of this change could be valuable to users.I've added the gnomonic projection. It's just ported from Karney's implementation.
However, I couldn't find any more test cases for the projection. Should I generate some random ones using example-Gnomonic.cpp?