-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
Conversation
Co-authored-by: ahuang11 <[email protected]>
Co-authored-by: ahuang11 <[email protected]>
Co-authored-by: ahuang11 <[email protected]>
|
I forgot a comment. Could you please document this feature? I think a simple mention in the |
|
Okay added docs |
hvplot/util.py
Outdated
| ) from Exception(*errors) | ||
|
|
||
|
|
||
| def is_within_latlon_bounds(data, x, y): |
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.
Can you please make these functions in util.py private by prepending their name with an underscore? That's the pattern I have adopted for new functions.
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.
Hmm, I'm on the fence about making them private. Maybe is_within_latlon_bounds but others I can see people calling it, e.g. from holoviews.util.transform import lon_lat_to_easting_northing. I typically reserve underscores for private methods, less utils, or else it should be a private method?
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.
I've made them private in my last commits. I've got way too much burnt by all the public-but-not-really utilities exposed in Param that need long deprecation cycles to be touched even for the smallest change. If we have a very useful utility that makes sense for hvPlot to expose, then it's easy to go from private to public, the other way is a chore.
hvplot/util.py
Outdated
| max_x = np.max(data[x]) | ||
| min_y = np.min(data[y]) | ||
| max_y = np.max(data[y]) | ||
| except (KeyError, ValueError, TypeError): |
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.
What cases do these exceptions cover?
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.
I think the goal was for it to gracefully fail so this auto-conversion doesn't block the user for anything unexpected--now I simply just use Exception and warn on exception.
| "id": "8c428430", | ||
| "metadata": {}, | ||
| "source": [ | ||
| "If `xlim` and `ylim` are in `lat`/`lon` coordinates, they will be automatically transformed to Web Mercator coordinates." |
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 sure we should document things this way in the gallery. Which is why I suggested doing it in the tiles example section, as this behavior is only enabled when tiles is enabled. For this example, I simply suggest updating the original snippet with xlim=..., ylim=....
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.
Thanks for clarifying, that makes sense. I wasn’t really sure what “a real example in the tiles section” referred to since I haven't worked in hvplot docs for a while, so just a heads up a quick link to the hvPlot docs section would be helpful for future comments / contributors! 🙇🏻
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.
Good point! 👍
|
@ahuang11 sorry I thought I submitted my review yesterday, but no! |
hvplot/util.py
Outdated
|
|
||
|
|
||
| def _convert_latlon_to_mercator(lon: np.ndarray, lat: np.ndarray): | ||
| def _convert_latlon_to_mercator(lon: np.ndarray, lat: np.ndarray, lon_180: bool = True): |
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.
I think it would read better if this is use_lon_180 or similar.
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.
Thanks, done in 43d75a7
|
The main changes I've pushed are in c76976b where |
Tested:
First is with xlim and ylim, 2nd is xlim, 3rd is ylim, 4th is none specified
Description
This PR implements automatic conversion of
xlimandylimparameters to Web Mercator (EPSG:3857) whentiles=Trueis used withoutgeo=True, making the behavior consistent with how data coordinates are automatically converted.Problem
Previously, when using
tiles=Truewith geographic data (lat/lon coordinates), hvPlot would automatically convert the data coordinates to Web Mercator, butxlimandylimparameters were not converted. This meant users had to manually calculate Web Mercator values for axis limits, which was inconvenient and error-prone:Solution
Now
xlimandylimare automatically converted to Web Mercator when appropriate, just like the data coordinates:Implementation Details
The conversion logic:
Checks data bounds first: Before converting
xlim/ylim, the code checks if the data coordinates are within typical lat/lon bounds (-180 <= lon <= 360,-90 <= lat <= 90). This prevents false positives where small numerical values might be within those ranges but represent non-geographic data.Converts only when appropriate: Conversion happens only when:
tiles=Trueis setgeo=False(not using explicit geo mode)projectionis notFalse(automatic projection not disabled)xlim/ylimvalues are within lat/lon boundsUses proper projection math:
easting = longitude × origin_shift / 180northing = log(tan((90 + latitude) × π / 360)) × origin_shift / πExamples
Geographic data (automatic conversion):
Non-geographic data (no conversion):
Testing
Added comprehensive tests covering:
xlimor onlyylim)projection=Falsedisables conversionAll tests pass and the implementation is fully backwards compatible with existing behavior.
Closes
Fixes the issue described in the problem statement where xlim/ylim were not automatically converted to Web Mercator when using tiles with lat/lon data.
Original prompt
Fixes #1617
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.