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

Add gnomonic projection #42

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add gnomonic projection #42

wants to merge 2 commits into from

Conversation

ondt
Copy link

@ondt ondt commented Oct 21, 2021

  • I agree to follow the project's code of conduct.
  • I added an entry to 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?

@michaelkirk
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Oct 21, 2021
@bors
Copy link
Contributor

bors bot commented Oct 21, 2021

try

Build succeeded:

Copy link
Member

@michaelkirk michaelkirk left a 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.

@ondt
Copy link
Author

ondt commented Oct 21, 2021

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

One approach:

  • Store some test inputs along with their respective Karney-approved outputs in an external file.
  • Have a test harness method that checks all these inputs

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

ondt commented Oct 22, 2021

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

michaelkirk commented Oct 22, 2021

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:

  • write a script/tool to generate test input assuming GeodesicProj is in the users path (or hardcode a path, whatever's easier for you) - have it live in this repo for now.
  • generate a test_fixtures/GnomTest-100.dat with a 100 test cases and check it in
  • 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.
  • update script/download-test-data.sh to download those files
  • like we've done with the geodesic math, have the tests run the in-repo tests (Gnom-100) input by default, but run the larger test inputs if given --features test_full or --features test_short.

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

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

michaelkirk commented Oct 22, 2021

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

ondt commented Oct 22, 2021

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

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.

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

Successfully merging this pull request may close these issues.

2 participants