search

georust / geographiclib-rs

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

Add gnomonic projection #42

Open ondt opened 3 years ago

ondt commented 3 years ago

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?

michaelkirk commented 3 years ago

bors try

bors[bot] commented 3 years ago

try

Build succeeded:

ondt commented 3 years ago

Do you know how upstream tests this feature?

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?

michaelkirk commented 3 years ago

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: https://github.com/georust/geographiclib-rs/blob/master/script/download-test-data.sh

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?

ondt commented 3 years ago

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:

  1. Generate random test inputs (with a fixed seed) in a similar manner as described in https://zenodo.org/record/32470, in three different sizes: full, small, built-in.
  2. Use GeodesicProj from Karney's toolkit to obtain Karney-approved outputs.
  3. Save the generated data in test_fixtures/test_data_generated/GnomCoords.dat.
  4. Run the test harness method that checks all these inputs.

Steps 1..=3 could be performed by some semi-external script that would depend on the Karney's toolkit. However, I'm not sure where to put this generator script. Should it be in a separate crate? Or in a crate within a cargo workspace? Or in a file in the examples/ directory? Or something else entirely?

michaelkirk commented 3 years ago

Thank you for laying out an approach. It sounds basically good, but I'd prefer to simplify it a bit:

we might not want to just store a large test set somewhere

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.

Steps 1..=3 could be performed by some semi-external script that would depend on the Karney's toolkit.

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 GeodesicProj can be found on PATH. We can either follow up to make it fancier - like downloading and building karney's toolikit (or containerizing it). But until then, since we've saved the output, the only value of the script is as a kind of almost-executable documentation for where the test inputs came from if we ever need to regenerate.

Does all that make sense? Is that agreeable?

michaelkirk commented 3 years ago

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.

michaelkirk commented 3 years ago

generate a test_fixtures/test_data_unzipped/GnomTest-short.dat with 10k test cases and test_fixtures/test_data_unzipped/GnomTest.dat with 500k test cases, but don't check it in. Zip and upload it somewhere.

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.

ondt commented 3 years ago

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 rand or other dependencies) would be easier than trying to somehow connect it to the cargo project in this repository without messing things up. The library could eventually take care of compiling/downloading the toolkit, and could potentially be included as an optional dev-dependency in geographiclib-rs.

michaelkirk commented 3 years ago

However, I'd rather write the "script" in Rust than as a shell script

Provided it's a small number of lines of code, it should be easy to review, so that's fine with me.