upgrade to proj 8.2 #97

michaelkirk · 2021-12-14T23:48:51Z

Just a draft for now as I work through some outstanding issues...

I've made the update, and rebuilt containers, but I'm still seeing some mysterious failures...

✅ proj-sys on rust-1.57
✅ proj-sys on rust-1.52
✅ proj on rust-1.57
- ✅ proj w/ --features=bundle
- ✅ proj using prebuilt proj
⁉️ proj on rust-1.52
- ✅ proj w/ --features=bundle
- 🔥 proj using prebuilt proj

For some reason using the pre-built libproj seems to be causing some smallish discrepencies on older containers. TBH I suspect the base system rather than the actual rust version.

next step is to try to see if just doing an apt upgrade fixes anything...

https://github.com/georust/proj/actions/runs/1580354293

failures:
---- proj::test::test_inverse_projection stdout ----
thread 'proj::test::test_inverse_projection' panicked at 'assert_relative_eq!(t.x(), 0.43633200013698786)

    left  = 0.43636512759931073
    right = 0.43633200013698786

', src/proj.rs:1069:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- proj::test::test_london_inverse stdout ----
thread 'proj::test::test_london_inverse' panicked at 'assert_relative_eq!(t.x(), 0.0023755864830313977)

    left  = 0.002407322568220306
    right = 0.0023755864830313977
', src/proj.rs:1086:9

---- proj::test::test_projection stdout ----
thread 'proj::test::test_projection' panicked at 'assert_relative_eq!(t.x(), 500119.7035366755, epsilon = 1e-5)

    left  = 499972.70746607287
    right = 500119.7035366755

', src/proj.rs:1054:9


failures:
    proj::test::test_inverse_projection
    proj::test::test_london_inverse
    proj::test::test_projection

urschrei · 2021-12-15T00:30:14Z

Just to make sure I'm understanding this:
There are currently two possible sources of difference:

Container libraries
Rust version

And you're going to try to eliminate (1). That's quite a surprising source!

michaelkirk · 2021-12-15T00:33:05Z

Ha, your instincts are better than mine. I'm pretty sure that's not the issue after poking at it a bit.

michaelkirk · 2021-12-15T00:43:26Z

My kingdom for even a wild theory as to how this could be happening...

The "bundled_proj" script builds from: https://github.com/georust/proj/pull/97/files#diff-27d669a056374d86fef7e0627a9011d3ef7388e4c16e67a981b1bfb6cc83413cR84

And that seems to be working all around. 👍

The containers which are not using bundled_proj are using the system proj. But it's not installed from apt or anything. It's built from this (seemingly identical) source code:

built here:
https://github.com/georust/docker-images/blob/mkirk/update-proj/rust-1.52/libproj-builder.Dockerfile

and copied here:
https://github.com/georust/docker-images/blob/mkirk/update-proj/rust-1.52/proj-ci.Dockerfile#L10

How would this be different?

urschrei · 2021-12-15T00:52:23Z

(Again, just so I'm clear here) the staged build copies the binary? Where on earth could a difference be coming from?

michaelkirk · 2021-12-15T01:00:23Z

(Again, just so I'm clear here) the staged build copies the binary? Where on earth could a difference be coming from?

That's right. In a little more detail:

The libproj-builder container compiles libproj (from the 8.2.0 source), but doesn't install it.

Containers which want libproj pre-installed (like proj-ci and geo-ci) copy it from the libproj-builder container into their own system path.

I can't figure out what the difference would be in the behavior of the proj compiled by the bundled_proj flag vs copied from the libproj-builder container.

michaelkirk · 2021-12-15T01:11:09Z

even wilder:

So when running all tests, three fail.

rust-1.52> cargo test
failures:
    proj::test::test_inverse_projection
    proj::test::test_london_inverse
    proj::test::test_projection

But when I run the failed tests indivudally:

rust-1.52> cargo test proj::test::test_inverse_projection
running 1 test
test proj::test::test_inverse_projection ... ok

passes!

rust-1.52> cargo test proj::test::test_london_inverse
running 1 test
test proj::test::test_london_inverse ... ok

passes!

rust-1.52> cargo test test_projection
test proj::test::test_projection ... ok

passes!

michaelkirk · 2021-12-15T01:23:04Z

Don't feel like you have to spend time on this, I'll keep plugging away, just documenting my findings as I go.

One more wild one... when running these two tests repeatedly, sometimes one fails, sometimes the other. It seems like there is some weird external effect at play, like some kind of race condition.

rust-1.52> cargo test projection
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests (/tmp/cargo-target/debug/deps/proj-ec6d6ab38686495b)

running 2 tests
test proj::test::test_inverse_projection ... ok
test proj::test::test_projection ... FAILED

failures:

---- proj::test::test_projection stdout ----
thread 'proj::test::test_projection' panicked at 'assert_relative_eq!(t.x(), 500119.7035366755, epsilon = 1e-5)

    left  = 499972.70746607287
    right = 500119.7035366755

', src/proj.rs:1054:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    proj::test::test_projection

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 13 filtered out; finished in 0.01s

root@6009bf331ef0:/tmp/georust/proj# /usr/local/cargo/bin/cargo test projection
    Finished test [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests (/tmp/cargo-target/debug/deps/proj-ec6d6ab38686495b)

running 2 tests
test proj::test::test_projection ... ok
test proj::test::test_inverse_projection ... FAILED

failures:

---- proj::test::test_inverse_projection stdout ----
thread 'proj::test::test_inverse_projection' panicked at 'assert_relative_eq!(t.x(), 0.43633200013698786)

    left  = 0.43636512759931073
    right = 0.43633200013698786

', src/proj.rs:1069:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    proj::test::test_inverse_projection

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 13 filtered out; finished in 0.01s

Also maybe/maybe not interesting, when I specify test-threads=1, test still fail, but they seem to follow a slightly different pattern.

vague theories:

some side effect caching bug?
maybe something in the network grid?

There's also the risk of some kind of memory corruption thing, but if that were the case, I might expect the incorrect values to be less close and/or less consistent.

urschrei · 2021-12-15T01:23:41Z

Oh no. What happens when you run two of them together, or add another conversion test? Because that sounds like memory might be getting clobbered by an FFI error somewhere…

urschrei · 2021-12-15T01:26:50Z

The network grid seems less likely, if only because we throw errors if it doesn't work. I guess an easy way to confirm that is to add a test that doesn't rely on the network and see if it fails in the same way?

urschrei · 2021-12-15T01:31:01Z

Re FFI: the issue causing #76 has been in the back of my mind.

michaelkirk · 2021-12-15T22:39:03Z

Re FFI: the issue causing #76 has been in the back of my mind.

Can you refresh my memory - what was the issue that lead to #76?

urschrei · 2021-12-15T23:05:02Z

Ugh sorry, I was thinking of #75, though they are related.

michaelkirk · 2021-12-16T00:06:44Z

Continuing to noodle on this...

Just as a sanity check...

    // Carry out a projection from geodetic coordinates
    fn test_projection() {
        let stereo70 = Proj::new(
            "+proj=sterea +lat_0=46 +lon_0=25 +k=0.99975 +x_0=500000 +y_0=500000
            +ellps=krass +towgs84=33.4,-146.6,-76.3,-0.359,-0.053,0.844,-0.84 +units=m +no_defs",
        )
        .unwrap();
        // Geodetic -> Pulkovo 1942(58) / Stereo70 (EPSG 3844)
        let t = stereo70
            .project(MyPoint::new(0.436332, 0.802851), false)
            .unwrap();
        assert_relative_eq!(t.x(), 500119.7035366755, epsilon = 1e-5);
        assert_relative_eq!(t.y(), 500027.77901023754, epsilon = 1e-5);
    }

This is an invalid coordinate, right?

MyPoint::new(0.436332, 0.802851)

Since it's a geodetic coordinate, shouldn't y always be less than 0.5 radians?

urschrei · 2021-12-16T00:22:06Z

I don't think so…you can readily convert it to degrees, which is a valid point in Romania (which is expected, given the Stereo70 projection)

michaelkirk · 2021-12-16T00:28:58Z

I don't think so…you can readily convert it to degrees, which is a valid point in Romania (which is expected, given the Stereo70 projection)

Ugh, of course. Sorry just confusing myself with really basic stuff.

On the plus side, I seem to have gotten tests passing locally with fb89caf

I'm not 100% sure on exactly what's happening, but I'm continuing to investigate.

Let's see what CI says...

michaelkirk · 2021-12-16T00:29:45Z

src/proj.rs

@@ -632,21 +632,30 @@ impl Proj {
        };
        let c_x: c_double = point.x().to_f64().ok_or(ProjError::FloatConversion)?;
        let c_y: c_double = point.y().to_f64().ok_or(ProjError::FloatConversion)?;
+        let coord = if inverse {
+            // Converting from degrees


whoops, got my comment backwards. 🙃

michaelkirk · 2021-12-16T00:43:39Z

Ok, it seems to be passing CI now.

Honestly, I think the change is somewhat "intuitive", in that if we're passing in degrees to project we use PJ_LP, and if we're doing an inverse projection, then we'd use PJ_XY to denote a projected Point (e.g. in meters).

But I confess I have no idea why this fixes anything! It's just a regular ole dumb C-union, so the representation of the PJ_XY vs PJ_LP variants of PJ_COORD should be identical.

... right?

michaelkirk · 2021-12-16T00:44:37Z

(disclaimer: I'm not well versed on c memory layout)

urschrei · 2021-12-16T00:44:58Z

🟢!
How did you decide to try that particular refactor?

urschrei · 2021-12-16T00:48:34Z

Ok, it seems to be passing CI now.

Honestly, I think the change is somewhat "intuitive", in that if we're passing in degrees to project we use PJ_LP, and if we're doing an inverse projection, then we'd use PJ_XY to denote a projected Point (e.g. in meters).

But I confess I have no idea why this fixes anything! It's just a regular ole dumb C-union, so the representation of the PJ_XY vs PJ_LP variants of PJ_COORD should be identical.

... right?

That's also my understanding – when I originally implemented it it looked like a simple union so I just shrugged and moved on, but I didn't look at the proj source to see whether it was doing anything different based on whether it was PJ_XY or PJ_LP…

michaelkirk · 2021-12-16T00:48:35Z

How did you decide to try that particular refactor?

Only intuition after reading PJ_COORD and its various flavors.

My read was that PJ_LP was for angular coords and PJ_XY was for projected coords - it seemed like we were using PJ_LP always.

But as I said above, I don't actually understand why this fixes anything, because my understanding is that their memory layout should be identical.

I'm going to read some proj source and try to learn something more about c unions...

michaelkirk · 2021-12-16T00:49:44Z

on whether it was PJ_XY or PJ_LP…

But as I understand it, it's equally both of those things. A basic union (as opposed to a tagged union) has no notion of what variant it is it just relies on the programmer to handle that.

urschrei · 2021-12-16T00:54:17Z

on whether it was PJ_XY or PJ_LP…

But as I understand it, it's equally both of those things. A basic union (as opposed to a tagged union) has no notion of what variant it is it just relies on the programmer to handle that.

I am out over my skis if that isn't the case, but given that it's just fixed what we suspected was a threading-related bug…

michaelkirk · 2021-12-17T01:17:47Z

Just by way of an update:

The tests are still occasionally failing for me, but seemingly much less with the changes around LP vs. XY. (I intermittently get failing results)

I feel pretty confident that this change is inducing different behavior, but not in any way that makes sense. I assume there is still some yet to be understood bug (presumably in our code) and that this change has just jostled the machine code around in such a way that we're avoiding the bug more often.

I'll keep looking into this, but maybe not for a couple days.

michaelkirk · 2021-12-22T19:05:54Z

I wonder if maybe it's static vs. dynamic linking that's causing the bug to present vs. not.

That could explain why --features bundled_proj (which statically links libproj) could succeed while linking to the system build (which links dynamically if available) of the exactly same build of proj (e.g. on proj-ci) could intermittently fail.

It doesn't help fix the bug, but at least it's a theory for how the two scenarios could be behaving differently.

michaelkirk · 2022-02-04T02:55:26Z

bors try

bors · 2022-02-04T03:08:58Z

try

Build failed:

ci result

michaelkirk · 2022-03-16T05:44:53Z

superseded by #118

118: update libproj to 9.0.0 r=michaelkirk a=michaelkirk - Fixes #96 - supersedes #97 Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

michaelkirk force-pushed the mkirk/proj-8.2 branch from 09be782 to 52d5cb6 Compare December 14, 2021 23:58

michaelkirk commented Dec 16, 2021

View reviewed changes

michaelkirk mentioned this pull request Feb 1, 2022

some failing tests #99

Closed

bors bot added a commit that referenced this pull request Feb 4, 2022

Try #97:

9a65886

michaelkirk mentioned this pull request Feb 4, 2022

Initialize zt before calling proj_trans #104

Merged

michaelkirk force-pushed the mkirk/proj-8.2 branch from ebef601 to eea0065 Compare February 23, 2022 22:08

michaelkirk added 2 commits March 8, 2022 17:06

update libproj to 8.2.0

21b0247

update ci containers

bed17eb

michaelkirk force-pushed the mkirk/proj-8.2 branch from eea0065 to bed17eb Compare March 9, 2022 01:09

michaelkirk mentioned this pull request Mar 16, 2022

update libproj to 9.0.0 #118

Merged

michaelkirk closed this Mar 16, 2022

bors bot added a commit that referenced this pull request Mar 21, 2022

Merge #118

d5db749

118: update libproj to 9.0.0 r=michaelkirk a=michaelkirk - Fixes #96 - supersedes #97 Co-authored-by: Michael Kirk <michael.code@endoftheworl.de>

michaelkirk deleted the mkirk/proj-8.2 branch April 11, 2022 19:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upgrade to proj 8.2 #97

upgrade to proj 8.2 #97

michaelkirk commented Dec 14, 2021 •

edited

Loading

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021

urschrei commented Dec 15, 2021

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021 •

edited

Loading

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021 •

edited

Loading

michaelkirk commented Dec 16, 2021

michaelkirk Dec 16, 2021

michaelkirk commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021

urschrei commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021

michaelkirk commented Dec 17, 2021 •

edited

Loading

michaelkirk commented Dec 22, 2021 •

edited

Loading

michaelkirk commented Feb 4, 2022

bors bot commented Feb 4, 2022

michaelkirk commented Mar 16, 2022

upgrade to proj 8.2 #97

upgrade to proj 8.2 #97

Conversation

michaelkirk commented Dec 14, 2021 • edited Loading

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021

urschrei commented Dec 15, 2021

urschrei commented Dec 15, 2021

michaelkirk commented Dec 15, 2021

urschrei commented Dec 15, 2021 • edited Loading

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021 • edited Loading

michaelkirk commented Dec 16, 2021

michaelkirk Dec 16, 2021

Choose a reason for hiding this comment

michaelkirk commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021

urschrei commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

michaelkirk commented Dec 16, 2021

urschrei commented Dec 16, 2021

michaelkirk commented Dec 17, 2021 • edited Loading

michaelkirk commented Dec 22, 2021 • edited Loading

michaelkirk commented Feb 4, 2022

bors bot commented Feb 4, 2022

try

michaelkirk commented Mar 16, 2022

michaelkirk commented Dec 14, 2021 •

edited

Loading

urschrei commented Dec 15, 2021 •

edited

Loading

urschrei commented Dec 16, 2021 •

edited

Loading

michaelkirk commented Dec 17, 2021 •

edited

Loading

michaelkirk commented Dec 22, 2021 •

edited

Loading