-
-
Notifications
You must be signed in to change notification settings - Fork 113
Automatically convert xlim/ylim to Web Mercator when tiles=True #1685
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
Open
Copilot
wants to merge
12
commits into
main
Choose a base branch
from
copilot/automatically-convert-xlim-ylim
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
289c5c1
Initial plan
Copilot be6a030
Add automatic xlim/ylim conversion to Web Mercator when tiles=True
Copilot faf4428
Add comprehensive tests for xlim/ylim conversion scenarios
Copilot d1d6e6b
Fix xlim/ylim conversion to check data bounds first
Copilot f55a2e4
clean up and tests
ahuang11 3db249d
clean up and fixes
ahuang11 5f503b5
remove dup check
ahuang11 de2de9d
reduce call
ahuang11 5f4f4a8
Add doc
ahuang11 51026d6
address comments
ahuang11 fa13680
add tests and tweak
ahuang11 03d6021
Merge branch 'main' into copilot/automatically-convert-xlim-ylim
ahuang11 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| import pytest | ||
|
|
||
| from hvplot.util import is_geodataframe | ||
| from holoviews.util.transform import lon_lat_to_easting_northing | ||
|
|
||
| try: | ||
| import dask.dataframe as dd | ||
|
|
@@ -30,6 +31,29 @@ def simple_df(): | |
| return pd.DataFrame(np.random.rand(10, 2), columns=['x', 'y']) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def lat_lon_df(): | ||
| return pd.DataFrame( | ||
| { | ||
| 'lon': [-120.0, -100.0, -80.0], | ||
| 'lat': [30.0, 35.0, 40.0], | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mercator_df(): | ||
| x_merc, y_merc = lon_lat_to_easting_northing( | ||
| np.array([-120.0, -100.0, -80.0]), np.array([30.0, 35.0, 40.0]) | ||
| ) | ||
| return pd.DataFrame( | ||
| { | ||
| 'x': x_merc, | ||
| 'y': y_merc, | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| class TestAnnotationNotGeo: | ||
| def test_plot_tiles_doesnt_set_geo(self, simple_df): | ||
| plot = simple_df.hvplot.points('x', 'y', tiles=True) | ||
|
|
@@ -104,3 +128,84 @@ def test_plot_without_crs(self): | |
| assert isinstance(plot.get(1), hv.Polygons) | ||
| bk_plot = bk_renderer.get_plot(plot) | ||
| assert bk_plot.projection == 'mercator' # projection enabled due to `tiles=True` | ||
|
|
||
| def test_xlim_ylim_conversion_with_tiles(self, simple_df): | ||
| """Test that xlim and ylim are automatically converted to Web Mercator when tiles=True""" | ||
| df = pd.DataFrame( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest using a fixture instead of repeating in each function the same DataFrame definition. |
||
| {'lon': [-120.0, -100.0, -80.0], 'lat': [30.0, 35.0, 40.0], 'value': [1, 2, 3]} | ||
| ) | ||
| plot = df.hvplot.points('lon', 'lat', tiles=True, xlim=(-130, -70), ylim=(25, 45)) | ||
| points = plot.get(1) | ||
|
|
||
| assert 'x' in points.data.columns | ||
| assert 'y' in points.data.columns | ||
|
|
||
| xlim_expected_0, _ = lon_lat_to_easting_northing(-130, 0) | ||
| xlim_expected_1, _ = lon_lat_to_easting_northing(-70, 0) | ||
| _, ylim_expected_0 = lon_lat_to_easting_northing(0, 25) | ||
| _, ylim_expected_1 = lon_lat_to_easting_northing(0, 45) | ||
|
|
||
| bk_plot = bk_renderer.get_plot(plot) | ||
| x_range_start = bk_plot.handles['plot'].x_range.start | ||
| x_range_end = bk_plot.handles['plot'].x_range.end | ||
| y_range_start = bk_plot.handles['plot'].y_range.start | ||
| y_range_end = bk_plot.handles['plot'].y_range.end | ||
| assert x_range_start == xlim_expected_0 | ||
| assert x_range_end == xlim_expected_1 | ||
| assert y_range_start == ylim_expected_0 | ||
| assert y_range_end == ylim_expected_1 | ||
|
|
||
| def test_xlim_only_conversion_with_tiles(self, lat_lon_df): | ||
| """xlim should convert even when ylim is not provided.""" | ||
|
|
||
| plot = lat_lon_df.hvplot.points('lon', 'lat', tiles=True, xlim=(-130, -70)) | ||
| bk_plot = bk_renderer.get_plot(plot) | ||
|
|
||
| x_start = bk_plot.state.x_range.start | ||
| x_end = bk_plot.state.x_range.end | ||
|
|
||
| np.testing.assert_almost_equal(x_start, -14471533.803125564) | ||
| np.testing.assert_almost_equal(x_end, -7792364.355529151) | ||
| assert x_start < x_end | ||
|
|
||
| def test_ylim_only_conversion_with_tiles(self, lat_lon_df): | ||
| """ylim should convert even when xlim is not provided.""" | ||
| plot = lat_lon_df.hvplot.points('lon', 'lat', tiles=True, ylim=(25, 45)) | ||
| bk_plot = bk_renderer.get_plot(plot) | ||
|
|
||
| y_start = bk_plot.state.y_range.start | ||
| y_end = bk_plot.state.y_range.end | ||
|
|
||
| assert y_start > 2_000_000 | ||
| assert y_end > 5_000_000 | ||
| assert y_start < y_end | ||
|
|
||
| def test_xlim_ylim_not_converted_without_tiles(self, lat_lon_df): | ||
| """Test that xlim and ylim are NOT converted when tiles=False""" | ||
| plot = lat_lon_df.hvplot.points('lon', 'lat', xlim=(-130, -70), ylim=(25, 45)) | ||
| bk_plot = bk_renderer.get_plot(plot) | ||
|
|
||
| x_range_start = bk_plot.handles['plot'].x_range.start | ||
| x_range_end = bk_plot.handles['plot'].x_range.end | ||
| y_range_start = bk_plot.handles['plot'].y_range.start | ||
| y_range_end = bk_plot.handles['plot'].y_range.end | ||
|
|
||
| assert x_range_start == -130 | ||
| assert x_range_end == -70 | ||
| assert y_range_start == 25 | ||
| assert y_range_end == 45 | ||
|
|
||
| def test_xlim_ylim_out_of_bounds_not_converted(self, mercator_df): | ||
| """Test that xlim and ylim are NOT converted when values are outside lat/lon bounds""" | ||
| plot = mercator_df.hvplot.points('x', 'y', tiles=True, xlim=(1000, 3000), ylim=(400, 800)) | ||
| bk_plot = bk_renderer.get_plot(plot.get(1)) | ||
|
|
||
| x_range_start = bk_plot.handles['plot'].x_range.start | ||
| x_range_end = bk_plot.handles['plot'].x_range.end | ||
| y_range_start = bk_plot.handles['plot'].y_range.start | ||
| y_range_end = bk_plot.handles['plot'].y_range.end | ||
|
|
||
| assert x_range_start == 1000 | ||
| assert x_range_end == 3000 | ||
| assert y_range_start == 400 | ||
| assert y_range_end == 800 | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
While we're at it, maybe we can do better than that here with a small utility function that finds a field name not already used (e.g. appending underscores until it finds it's available). In this case we don't check if
'x_'is already used or not.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.
Not too convinced we need this; I personally have not encountered coordinates with
x_in my experience.