search

georust / geographiclib-rs

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

hook up CI #17

Closed michaelkirk closed 3 years ago

michaelkirk commented 3 years ago

Finishes #13 by hooking up CI.

By default cargo test will run against the short data set. cargo test --features full_test will run agains the long one.

CI will run full_test (full_test is maybe a bit too slow to run all the time for interactive development feedback, but it's not too long for CI).

michaelkirk commented 3 years ago

The new CI has passed! Please take a look @stonylohr

michaelkirk commented 3 years ago

bors try

... lets see if bors got hooked up correctly.

bors[bot] commented 3 years ago

try

Build succeeded:

michaelkirk commented 3 years ago

I've pushed up a change so that, by default, the tests can run without downloading any additional data. Would hate to have the tests fail on first download for folks.

Any feedback on this before I merge @stonylohr?

stonylohr commented 3 years ago

@michaelkirk I don't see any obvious issues in a naive review, but it's using a few tricks I don't know very well, and I lack time to try pulling and running it now. In principle, I really like your approach of defaulting to minimal, while offering the other options, and scripting the data download. In terms of a simple baseline, the unit tests feel pretty good to me at this point.

I would favor also expanding the benchmarks before starting on troubleshooting the core code. This might boil down to offering the same builtin/small/full options on the rs benches that you added to the unit tests. I suggest replacing the benches' current data-copying approach with Arc+Mutex at the same time. That improved speed by about 66% in some benches I compared, though it may depend in part on the size of the test dataset. Maybe the bench changes should be a separate PR, though? I'm fine either way.

michaelkirk commented 3 years ago

bors r+

Thanks for looking @stonylohr.

I would favor also expanding the benchmarks before starting on troubleshooting the core code

Seems like a good idea. I've created a new issue for that https://github.com/georust/geographiclib-rs/issues/19

bors[bot] commented 3 years ago

Build succeeded: