-
Notifications
You must be signed in to change notification settings - Fork 364
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
Bump proj4 version to fix multiple performance issues #3039
Bump proj4 version to fix multiple performance issues #3039
Conversation
4ff1791
to
5019454
Compare
5019454
to
77dc77e
Compare
77dc77e
to
6490327
Compare
new CRS { | ||
val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params) | ||
} | ||
new CRS { val proj4jCrs: CoordinateReferenceSystem = crsFactory.createFromParameters(name, proj4Params) } |
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.
In proj4 1.1 or 2.0 (whatever), can we consider making these case
classes instead of anonymous ones? Make for a bit more natural serialization, easier debugging, etc.
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.
@metasim it would be a geotrellis change, sure I'll do it 👍
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.
@metasim do you have any preferred idea / class hierarchy for the entire CRS story?
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.
@pomadchin I hadn't thought about it until you asked... how's this for a strawdog:
When fromWKT
or fromProj4String
or getEpsgCode
you'd get an instance of one of the intermediate classes.
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.
@metasim can you bring this conversation into a separate issue?
More better CRS equality from Proj4j 1.1.0-SNAPSHOT revealed this problem and after some digging I conclude that the test is wrong.
Last commit pushed in these changes to address the test failures due to bad equality locationtech/proj4j#45 If this PR goes green I'm OK having |
Also a note benchmarks are the same, so @echeipesh is a maestro. |
Overview
This PR updates proj4 version up to locationtech/proj4j#33
Checklist
docs/CHANGELOG.rst
updated, if necessaryNotes
Benchmarking results:
Closes #2890
Closes #3022
@metasim probably you're interested in this PR