-
Notifications
You must be signed in to change notification settings - Fork 205
Add handling of GeoDoubleParams, GeoAsciiParams to GeoKeyDirectory #477
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
base: master
Are you sure you want to change the base?
Conversation
Took another pass and decided to process the I'm not entirely sure how to handle the case when the user wants to provide any one of |
@DanielJDufour Could you take a look please? |
Thanks for forwarding! Note I used the spec, and useful example from maptools doc to build the |
Hi, I'm reviewing this now. |
test/geotiff.spec.js
Outdated
const image = await tiff.getImage(); | ||
|
||
const geoKeys = image.getGeoKeys(); | ||
expect(geoKeys.GeogSemiMajorAxisGeoKey).to.be.closeTo(6378137.0, 0.000001); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jo-chemla , could you provide a little more background on why this was necessary? I'm assuming it's a floating-point arithmetic thing, but would love to know a little more details. What part of the code do you think is causing this? Thank you!
on somewhat personal note: I've spent more time than I care to admit dealing with floating point arithmetic issues in the geo space, so I prefer it there's a way to preserves numbers while round-tripping that would be awesome!! It'll definitely save later downstream confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was actually not necessary, just switched these lines from closeTo
to equal
. JS only uses double precision numbers, so it indeed looks like round-tripping does preserve these numbers!
expect(geoKeys.GeogSemiMajorAxisGeoKey).to.equal(6378137.0);
Hey, @jo-chemla . This PR looks awesome!! It definitely improves the writing functionality a lot. I just left one minor question/comment. Just double checking on something. |
Thanks for getting back and the comment, I've resolved it - you were right, the test can be a simple equal even for the double geoKeys. Should be ready to merge from my point-of-view! |
Approved! |
Fixes #476 (comment)
This GeoTIFF spec by maptools was a very important resource to understand these ascii and double tags and how they relate to
GeoKeyDirectory
Note I have added both
GeogCitationGeoKey
gets parsedAlso, I later decided to build the offsets object when adding type-filtered keys to the
GeoDoubleParams, GeoAsciiParams
structures, to avoid recomputing them afterwards when adding toGeoKeyDirectory.
EDIT: also managed to import test-data - on a windows machine, had to rework some wget/unzip and gdal commands - and added a test to geotiffwriter spec, only for the case I added of DOUBLE/ASCII parsing.