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

ENH: Added allow_superseded kwargs to TransformerGroup #1269

Merged
merged 2 commits into from
Apr 11, 2023
Merged

ENH: Added allow_superseded kwargs to TransformerGroup #1269

merged 2 commits into from
Apr 11, 2023

Conversation

dmahr1
Copy link
Contributor

@dmahr1 dmahr1 commented Apr 9, 2023 •
edited
Loading

dmahr1
dmahr1 commented Apr 9, 2023
View reviewed changes
pyproj/_transformer.pyx Outdated Show resolved Hide resolved
dmahr1
dmahr1 commented Apr 9, 2023
View reviewed changes
pyproj/_transformer.pyx Outdated Show resolved Hide resolved
dmahr1
dmahr1 commented Apr 9, 2023
View reviewed changes
test/test_transformer.py Outdated Show resolved Hide resolved
dmahr1
dmahr1 commented Apr 9, 2023
View reviewed changes
test/test_transformer.py
Comment on lines +1661 to +1675
@pytest.mark.parametrize(
"from_crs, to_crs, allow_superseded, expected_num_transformers",
[
(6319, 5703, 0, 2),
(6319, 5703, False, 2),
(6319, 5703, True, 3),
(6319, 5703, 123456789, 3),
(6319, 5703, "blah", 3),
],
)
def test_transformer_group_allow_superseded_filter(
from_crs, to_crs, allow_superseded, expected_num_transformers
):
group = TransformerGroup(from_crs, to_crs, allow_superseded=allow_superseded)
assert len(group.transformers) == expected_num_transformers
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exercises the case from issue #1261.

@dmahr1 dmahr1 marked this pull request as ready for review April 9, 2023 02:44
snowman2
snowman2 reviewed Apr 9, 2023
View reviewed changes
pyproj/_transformer.pyx Outdated Show resolved Hide resolved
@dmahr1 dmahr1 force-pushed the dmahr_allow-superseded branch from 0e6eca4 to b31f272 Compare April 9, 2023 23:54
@codecov
Copy link

codecov bot commented Apr 10, 2023 •
edited
Loading

Codecov Report

Merging #1269 (c0b1755) into main (f376625) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1269   +/-   ##
=======================================
  Coverage   96.40%   96.40%           
=======================================
  Files          20       20           
  Lines        1810     1810           
=======================================
  Hits         1745     1745           
  Misses         65       65
Impacted Files Coverage Δ
pyproj/transformer.py 95.08% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@snowman2
Copy link
Member

This is looking good. @dmahr1, allow_superseded can be removed from Transformer.from_crs now, correct?

@dmahr1
Copy link
Contributor Author

dmahr1 commented Apr 10, 2023

This is looking good. @dmahr1, allow_superseded can be removed from Transformer.from_crs now, correct?

@snowman2 I was thinking of leaving the allow_superseded kwarg in Transformer.from_crs() so that superseded transformers could be included. But I have been unable to find a case where this is actually used. So I understand if you'd prefer to remove it.

What do you think?

@snowman2
Copy link
Member

I was thinking of leaving the allow_superseded kwarg in Transformer.from_crs() so that superseded transformers could be included. But I have been unable to find a case where this is actually used. So I understand if you'd prefer to remove it.

I recommend adding the option to the TransformerGroup now and then add it to Transformer.from_crs in a future PR once the feature has been added to a PROJ release. Otherwise, this PR will be blocked until the feature is added in a future PROJ release.

@dmahr1 dmahr1 changed the title ENH: Added allow_superseded kwargs to Transformer.from_crs and TransformerGroup ENH: Added allow_superseded kwargs to TransformerGroup Apr 10, 2023
@dmahr1
Copy link
Contributor Author

dmahr1 commented Apr 10, 2023

@snowman2 Ah, I must have misunderstood earlier, I assumed that the allow_superseded was already available for from_crs(). I removed that from my PR. It should only have the logic for TransformerGroup now.

@snowman2 snowman2 added this to the 3.6.0 milestone Apr 10, 2023
snowman2
snowman2 approved these changes Apr 10, 2023
View reviewed changes
Copy link
Member

@snowman2 snowman2 left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@snowman2 snowman2 merged commit e4d5abc into pyproj4:main Apr 11, 2023
@dmahr1 dmahr1 deleted the dmahr_allow-superseded branch April 11, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snowman2 snowman2 snowman2 approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.

Superseded vertical grid shifts not in TransformerGroup
2 participants
@dmahr1 @snowman2