-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adding support for EPSG:5070 and Albers projections generally #184
Conversation
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
Various code style changes, changing ^0.5 to sqrt, inlined small helper function, split vectorized array operations to convert degrees to radians into individual lines, fixed missing and redundant type parameters. Learning! Co-authored-by: Elias Carvalho <[email protected]>
Co-authored-by: Elias Carvalho <[email protected]>
removed divide by a since this is performed in another step, reordered constants, shifted constants so that all operations on \lambda and \phi occur within locally defined functions Co-authored-by: Elias Carvalho <[email protected]>
… computing each time, using auth2geod Co-authored-by: Elias Carvalho <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #184 +/- ##
==========================================
+ Coverage 97.29% 97.42% +0.13%
==========================================
Files 31 32 +1
Lines 1220 1284 +64
==========================================
+ Hits 1187 1251 +64
Misses 33 33 ☔ View full report in Codecov by Sentry. |
I'm going through the process of building the tests (my bad on not realizing the tests extended beyond get.jl) and I've noticed some issues with the code. I need to ensure that the degrees wrap (e.g. Additionally, while any lat lon can work with Albers, it's best practice to keep it between the standard parallels. Would it be fine to use lat_1 and lat_2 for the inbounds check? |
updated so that inbounds all longitude, only standard parallels latitude and \lambda properly wraps within the function test/crs/* added tests for Albers in each I COULD NOT TEST AS OF THIS MOMENT. ALL APPROX WASNT ACCEPTING THE METHOD, I BELIEVE IT HAS TO DO WITH THE FACT THE CRS IS SHIFTED I DID DETECT > 1M INACCURACY FROM PROJ. WILL HAVE TO INVESTIGATE Additionally, selected coordinates for tests are not final.
changed eqareaalbers to allow latitudes up to \pi and included a helper function to wrap latitudes if greater than abs(\pi/2) Fixed tests to properly construct an Albers object and test it Conversions needs more work. Some of the allapprox tests failed but have submeter inaccuracy. The real issue is in back transforming. Latitudes appear to be back transforming inappropriately, so that will have to be double checked
… shifts. tests work on my machine EXCEPT for conversions. Something is wrong with the backward latitude function. The forward function for T(45), -T(90) and -T(45), -T(90) don't pass tests. Will have to investigate further. Reverted change to fy(), \rho probably isn't going to be <0 zero with my wrapping scheme.
… pass on my machine except for conversion tests 3 and 4. The forward calculation diverges from PROJ at sub meter
…ll do more checks later today), and removed the default constructor function Co-authored-by: Elias Carvalho <[email protected]>
…. Updated tests to use the results from Proj.jl and EPSG:4269 (previously used general Proj with EPSG:4326). changed \Theta's to \theta to reflect documentation. updated backwards \theta to switch signs of arguments if n <0 based on latest EPSG guidance note.
…fSystems.jl into JC_add5070_albers
@souma4, I made some adjustments to your PR, I hope I helped. |
You have helped immensely. Got me more in the "developer/maintainer" mindset which is a good experience. And I'll likely contribute to this broader ecosystem more. Thanks for bearing with my issues in the process, hah. |
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.
@souma4 this PR looks amazing. Thank you @eliascarv for adjusting it.
See #183. In short, adding support for EPSG: 5070 and added Albers projection into src/crs/projected/