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

Increase accuracy with omerc projection #317

Merged
merged 6 commits into from Jan 21, 2021

Conversation

dulldrums
Copy link
Contributor

NOTE:

This implementation is based off of the mapshaper-proj implementation of omerc, which is based directly off of the proj4 implementation of omerc. I attempted to match the structure of proj4js as best as possible.

Also, one test I added is failing. The test data I have added is coming from proj4, so it should be correct. It could indicate that there is more work to be done for this implementation.

This issue is mentioned in #308 , #313 , #301 , #273 , #308 , #173, and #146.

Problem:

The omerc projection is returning inaccurate values. As mentioned in #308, it appears as though the implementation is ignoring the +gamma value. Additionally, whenever the alpha values are +/-90, the transformed coordinates were inaccurate.

Solution:

I had found this implementation of omerc that appeared to be producing more accurate results than the proj4js implementation of omerc. There are several things in the mapshaper-proj implementation that are dependant on that library's infrastructure, so I tried to work around that as best as possible.

I would love to get a second set of eyes on this code to verify that this fix is actually helping.

There is currently a failing test that I have added. 12 tests are failing with this implementation, 32 are failing with the previous one.

I'll continue to investigate why this test is failing, and update this PR as soon as I figure it out.

Thanks!

@dulldrums
Copy link
Contributor Author

Here's a demo of something I threw together with the example from the issue here: #308 (comment)

It appears that the mapshaper-proj results match what is returned from proj4. (The version of proj4js used here is what is currently in this repository, not the implementation from my branch)

https://codesandbox.io/embed/r48wn6voq

@calvinmetcalf
Copy link
Member

ok so we need to test some of the failing tests to make sure that they are in fact real failures, when I created some of these tests I just plugged coordinates into this library and recorded their results and used those for the tests so that any refactoring we did wouldn't change the output, this doesn't necessarily mean they are correct though so if we plug the same projection and coordinates into a different library (or better yet 2) and they give a different result which is the same as the 'incorrect' error we get, we can probably just fix the test.

also the style is a bit wonky compared to other files but we don't have to worry about that until after we make sure it works.

lastly I just merged in #318 which fixes travis builds so you probably want to rebase off that

@dulldrums
Copy link
Contributor Author

@calvinmetcalf got it, thanks! I'll come up with a way to test all the values for omerc tests, and update them where it's necessary

@dulldrums
Copy link
Contributor Author

@calvinmetcalf Rebased and added a commit to fix those tests.

Also, added a test for the transformation @nethdeco described in their issue (#308).

The values I took for these tests are coming from proj4, but would love to have someone else verify that these values are accurate. I compared these values to values from QGIS, and noticed a slight difference for CH1903 / LV03 (EPSG:21781). It looks like this transformation isn't as accurate as the others being tested, but the difference seems greater than the others (e.g. EPSG:2057). This fix returns virtually the same values as the previous implementation (acc: { xy: 9}), so it makes me think the issue is with inconsistencies in the definition of EPSG:21781.

Please let me know if there is anything else for this PR.

Thanks!

@calvinmetcalf
Copy link
Member

i wonder if this fixes the issues we had with epsg 3468 ?

@dulldrums
Copy link
Contributor Author

Hmm, it doesn't look like it is... Ill have a quick look to see what it could be

@dulldrums
Copy link
Contributor Author

It looks like it might have something to do with the +no_uoff param being ignored. I compared this fix with mapshaper, and it looks like mapshaper is producing accurate results.

Looking at the Proj4 definition on http://epsg.io/3468, it appears the +no_uoff param is there. I found this issue linked to Proj4 OSGeo/PROJ#104 (comment) .

It looks like we need to do something like this somewhere: https://github.com/mbloch/mapshaper-proj/blob/master/src/wkt/projections/wkt_omerc.js#L8

@calvinmetcalf
Copy link
Member

I'm pretty sure it doesn't work if you if you use the WKT version of the proj definition as well, which doesn't have the no_uoff param at all

@dulldrums
Copy link
Contributor Author

I'm pretty sure it doesn't work if you if you use the WKT version of the proj definition as well, which doesn't have the no_uoff param at all

Exactly. I'm trying to figure out how to assume the value of no_uoff if it isn't provided. Haven't been able to figure it out yet though

@calvinmetcalf
Copy link
Member

it looks like mapshaper uses the name to differentiate it, which should be available in there somewhere

@@ -181,8 +181,8 @@ export function inverse(p) {
var u, v, Qp, Sp, Tp, Vp, Up;
var coords = {};

p.x = (p.x * this.to_meter - this.x0) * (1.0 / this.a);
p.y = (p.y * this.to_meter - this.y0) * (1.0 / this.a);
p.x = (p.x - this.x0) * (1.0 / this.a);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we going to need to add some tests for non meter omerc projections ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think so.

I'll get back to this PR sometime next week, got pulled into some other stuff.

@dulldrums
Copy link
Contributor Author

Just pushed a new commit up, a couple of things to note:

  • This change fixes the issue with EPSG:3468. Tests have been added to reflect that.
  • I couldn't find any valid proj4 string or WKT to test the projection "Hotine_Oblique_Mercator_Two_Point_Natural_Origin". Do you know any EPSG code that can be used to get the data to test this?
  • The logic to check if the omerc is type A or B is dependent on what Projection string is provided. I did my best to make this logic as clear as possible.
  • Tests were added for both Type A and Type B projections, using both Proj4 format and WKTs
  • I updated the EPSG:2057 test with accurate values.
  • I couldn't find any omerc projections that use ft, some tests for that could be added though

Let me know if there is anything else! Thanks!

@dulldrums
Copy link
Contributor Author

Just added a test for omerc type B using Foot_US as the unit

@calvinmetcalf
Copy link
Member

epsg:54025 should be Hotine_Oblique_Mercator_Two_Point_Natural_Origin, this is great!

@dulldrums
Copy link
Contributor Author

@calvinmetcalf I had seen EPSG:54025, but it doesn't seem to work on http://epsg.io/transform#s_srs=4326&t_srs=54025&x=1.0000000&y=1.0000000 or using mapshaper. I'm not sure if it is a problem with its definition.

@childsd3 childsd3 mentioned this pull request Apr 30, 2020
Copy link
Member

@ahocevar ahocevar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. @calvinmetcalf, do you want to take another look?

@ahocevar ahocevar merged commit 587ddd5 into proj4js:master Jan 21, 2021
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.

None yet

3 participants