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

Fix forcing zones around equator and add force_northern in from_latlon #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sdhiscocks
Copy link
Contributor

Logic in from_latlon has been changed to be similar to to_latlon, including adding a force_northern option to mirror northern option in to_latlon. This resolves the issue with northing coordinate incorrectly being set when passing in -ve or +ve latitude when forcing northern or southern zone respectively.

Closes #35 - alternative fix for issue.

@codecov
Copy link

codecov bot commented Mar 28, 2020

Codecov Report

Merging #46 (76d6ae1) into master (fed38ea) will increase coverage by 0.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #46      +/-   ##
==========================================
+ Coverage   95.09%   95.62%   +0.53%     
==========================================
  Files           3        3              
  Lines         163      160       -3     
==========================================
- Hits          155      153       -2     
+ Misses          8        7       -1     
Impacted Files Coverage Δ
utm/conversion.py 95.51% <100.00%> (+0.54%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fed38ea...76d6ae1. Read the comment docs.

@sdhiscocks
Copy link
Contributor Author

sdhiscocks commented Mar 28, 2020

Note that the tests have only failed on Python 2.6 and 3.3, as these versions of Python aren't available on TravisCI's default build machine anymore.

@sdhiscocks
Copy link
Contributor Author

I've rebased this branch to remove merge conflicts.

@sdhiscocks
Copy link
Contributor Author

Updated so tests cover entire diff (previously missing test for providing force_zone_letter and force_northern which raises ValueError)

Logic in from_latlon has been changed to be similar to to_latlon,
including adding a force_northern option to mirror northern option in
to_latlon. This resolves the issue with northing coordinate incorrectly
being set when pasing in -ve or +ve latitude when forcing northern or
southern zone respectively.

Closes Turbo87#35 - alternative fix for issue.
@sdhiscocks
Copy link
Contributor Author

Rebased, updating tests to pytest.

@sdhiscocks
Copy link
Contributor Author

@Turbo87 Just wondering if you had any thoughts on this PR.

@bartvanandel
Copy link
Collaborator

I do like the proposed changes in this branch, but there are some concerns which need to be addressed.

The current proposal removes some sanity checks for sets of points that check whether points (kind of) belong to the same zone. I think we should be careful about removing those (original comment). If it is desired by the user of our lib to treat lots of points as if they were in the same zone, we'd better be explicit about allowing that by introducing another option (original comment).

I may have a look at it later today. If I'm not quick enough, @heathhenley, would you mind having a go at it?

@heathhenley
Copy link
Contributor

@bartvanandel happy to help! Just to be clear that I'm on the same page, are you thinking that we should do something like

  • keep the new option introduced in the pr
  • also keep the sanity checks and defaults to existing behavior

Or are you thinking that we should add a new option with slightly different behavior?

@bartvanandel
Copy link
Collaborator

I think we're pretty much on the same page indeed. New option according to PR, but keep the sanity checks by default, unless explicitly told to ignore them. This way I think the default behavior makes the most sense.

@Turbo87 if you have any thoughts or objections, now is the time 😉

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.

3 participants