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.

Sign up for GitHub

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

Jump to bottom

Introduction of a projection parameter for convert.py functions and ISG grid/geo conversion capability #138

Merged
merged 7 commits into from
Feb 21, 2022
Merged

Introduction of a projection parameter for convert.py functions and ISG grid/geo conversion capability #138

merged 7 commits into from
Feb 21, 2022

Conversation

jonathonpsmith
Copy link
Contributor

Changes to various functions in convert.py to allow conversions by passing in an instance of a Projection object as a parameter.
Various changes to allow easy conversions between AGD66 and the ISG projection for NSW users.

  • Remove the hardcoded 'proj' variable from convert.py and introduce a 'prj' parameter in the following functions:
    • psfandgridconv()
    • geo2grid()
    • grid2geo()
  • Define new Projection instance 'isg' in constants.py
  • Add new functionality to calculate the zone and central meridian for ISG
  • Add new exceptions for invalid ISG zones (ISG is only valid in NSW)
  • Add a warning for non-recommended ellipsoid/projection combinations for ISG
  • Add new tests to test_convert.py to check the above new functionality.

jonathonpsmith and others added 5 commits February 10, 2022 16:00
@jonathonpsmith
define ISG as an instance of a Projection object
c88cb14
@jonathonpsmith
add prj as a parameter to:
740537b 
  - def psfandgridconv()
  - def geo2grid()
  - def grid2geo()
replace all references to 'proj' with function parameter 'prj' in the above functions.
add exception if ISG selected as projection without ANS as ellipsoid.
add to 'Invalid Zone' exceptions to include ISG zones.
add zone and CM calculations for ISG to def geo2grid
add CM calculations for ISG to def grid2geo
@jonathonpsmith
use the warnings module to warn about mismatch between ISG projection…
5de454e 
… and chosen ellipsoid, instead of raising an exception

add zone 572 (Lord Howe Island) to the allowable zones for ISG
@jonathonpsmith
add def test_geo_grid_interoperability_isg() to test ISG functionality
4aa33ab 
add tests to def test_geo2grid() and def test_grid2geo() to test:
  - ValueError raised for incorrect ISG zone
  - UserWarning raised for prj/ellipsoid combination for ISG that isn't recommended
@jonathonpsmith
Merge pull request #1 from jonathonpsmith/add_isg
7f99d6b 
Introduction of integrated projection handling and ISG grid/geo conversion capability
@nicgowans
Copy link
Contributor

More information on the Integrated Survey Grid projection and its historical implementation in NSW can be found at the NSW Spatial Services Map Projections page and in the ISG Technical Manual

@harry093
Copy link
Collaborator

Thanks @jonathonpsmith and @nicgowans, this looks really useful. I have just run the checks and all seems to be in order. If I don't get to this this afternoon, I will on Monday. I will push a new version to PyPI, too

@jonathonpsmith
Copy link
Contributor Author

Thanks Craig, sounds great. Glad I could be useful!

@BatchelorJ
Copy link
Collaborator

Hi @jonathonpsmith and @nicgowans, this looks really great! Happy that you've taken the projection concept to the next level. A couple of comments:

  • @harry093, as this change touches on some of the functions used in the GA geodesy apps, have these been tested and reviewed?
  • @jonathonpsmith, are you able to add a comment and link in geodepy.constants where isg is defined to help users find the references @nicgowans highlighted?

@jonathonpsmith
Copy link
Contributor Author

Hey @BatchelorJ, yep too easy.
I've added a description comment for ISG and both links that @nicgowans supplied, with a description of what each are.

I don't believe the projection parameter addition would affect any other functions, as it's defined with 'utm' as the default, which preserves the existing functionality.

harry093
harry093 reviewed Feb 21, 2022
View reviewed changes
Copy link
Collaborator

@harry093 harry093 left a comment

Choose a reason for hiding this comment

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

@jonathonpsmith these changes look all in order. My only suggestion is to reduce the number of tests. If there is a good reason for having so many, I am happy to leave them in.

@harry093
Copy link
Collaborator

as this change touches on some of the functions used in the GA geodesy apps, have these been tested and reviewed?

@BatchelorJ, I agree with @jonathonpsmith that these changes should not break anything as prj is an optional argument that has a default value (utm) defined.

To be safe, though, after these changes have been pushed through to PyPI, we can update the version of Geodesy on the dev branch and make sure everything is working correctly.

@harry093 harry093 merged commit 1149a66 into GeoscienceAustralia:master Feb 21, 2022
@harry093
Copy link
Collaborator

@jonathonpsmith @nicgowans @BatchelorJ - GeodePy v0.0.28 is now up on PyPI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harry093 harry093 harry093 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonathonpsmith @nicgowans @harry093 @BatchelorJ