Closed joshuanunn closed 4 years ago
urschrei
commented
4 years ago Oh, wow, thanks for catching this!
This arrangement gives much closer overall agreement with the OS test set.
Which test results are now improved? I assume you mean in test_os_etrs89_to_osgb36, but I'd like to be sure…
joshuanunn
commented
4 years ago Sorry, I should have been a bit more specific and yes, please make sure that you definitely agree first! Some of the base terminology is quite confusing, so I hope I've interpreted correctly...
Which test results are now improved? I assume you mean in
test_os_etrs89_to_osgb36, but I'd like to be sure…
It's actually kind of the reverse of this test, but I've only just realised the equivalent OS test set I'm referring to doesn't exist in your tests. I only noticed this because I've been porting this code to Julia (as mentioned recently) and I couldn't get the OS test set to pass when going from eastings,northings to lon,lat (with OSTN15). Due to me having some issues trying to get the rust source to build on my local machine, I've used the python wrapped convert_bng to make a few checks as I go. As such, what I've found is more testing by proxy, rather than based directly on the rust tests. I appreciate the python wrapped version may be a little behind the rust master here too.
In your rust test suite, you use the quoted example from the OS literature for the Caister Water Tower - i.e. you effectively test convert_osgb36_to_lonlat(651409.804, 313177.450) == ([1.71607397], [52.65800783]), which works fine with or without the change in this pull request. However, when testing the wider OS test set in the OSTN15-OSGM15-DevelopersPack, I think there are discrepancies. Specifically, this is when converting the coordinates in OSTN15_OSGM15_TestInput_OSGBtoETRS.txt to the ones in OSTN15_OSGM15_TestOutput_OSGBtoETRS.txt. To pick three examples from these files and comparing the outputs match to 0.0000001 d.p:
TP21 works (with and without proposed change):
convert_osgb36_to_lonlat(227778.330, 468847.388) gives:
"Rust" value: -4.63452169, 54.08666318 [OK]
Julia value: -4.63452169, 54.08666318 [OK]
OS value: -4.63452168212, 54.08666318080
TP32 fails:
convert_osgb36_to_lonlat(71713.131, 938516.405) gives:
"Rust" value: -7.59255577, 58.2126235 [Fails]
Julia value: -7.59255561, 58.21262247 [OK]
OS value: -7.59255560556, 58.21262247180
TP40 fails:
convert_osgb36_to_lonlat(71713.131, 938516.405) gives:
"Rust" value: -2.07382823, 60.13308136 [Fails]
Julia value: -2.07382823, 60.13308092 [OK]
OS value: -2.07382822798, 60.13308091660
In total [by proxy in Julia], without the proposed changes I get 15/40 tests fail for comparison at 0.0000001 d.p. (this is what you test at) and with the applied changes, all of these tests pass at the same precision. However, for most purposes I am not sure if the differences here are significant enough?
I guess really I should have added an additional test group here to accompany these changes, but I hadn't realised that the changes won't have been covered by the existing tests. I could try and do this, but I struggled to get the rust source working correctly on my system.
Any thoughts?
urschrei
commented
4 years ago I'm happy to add the missing tests from the developer pack – don't worry about it! Separately, What errors are you getting when trying to build from source? I know the initial ostn15_phf build can take a very long time, and there have been failures on 32-bit Windows targets in the past…
As in the OS documentation, tolerance should be set at 0.00001 m (0.01 mm) - this results in a greater number of iterative cycles here.
I believe the order of calculation of m and phi in the iterative steps is incorrect. In its current form, the latest calculated value of m in each iteration does not then get used in updating the current value of phi. This arrangement gives much closer overall agreement with the OS test set.