Fix DMS not set on some projections #293
Conversation
|
Some baselines will probable fail (but maybe not). Will add a test. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures degree-minute-second (DMS) formatting is consistently applied to projections that require it.
- Added a test to verify DMS formatting on the Mercator projection.
- Removed explicit gridline formatter overrides so axis formatters can propagate DMS formatting.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ultraplot/tests/test_geographic.py | Add test_dms_used_for_mercator to validate DMS formatting |
| ultraplot/axes/geo.py | Remove explicit gl.xformatter/gl.yformatter assignments |
Comments suppressed due to low confidence (2)
ultraplot/tests/test_geographic.py:864
- The new test only verifies longitude formatting (
xformatter). To fully cover DMS behavior, add assertions for latitude formatting usinggridlines_major.yformatteron thelatticks.
a = ax[0].gridlines_major.xformatter(tick)
ultraplot/axes/geo.py:1708
- By removing the explicit assignment of
gl.xformatterandgl.yformatter, gridlines may no longer pick up the DMS formatter from the axis. Consider reintroducing these assignments (after the axis formatter is configured) so gridlines use the correct DMS formatter.
)
beckermr
left a comment
beckermr
left a comment
There was a problem hiding this comment.
This change confuses me. There were no cases where the code that's been removed was needed?
|
Actually you are right, I thought I added this but that's not true will do some more searching why it is not working. |
|
It's a bit strange when I edit line 244 and remove the merc projector it is set to false and it works as it should for the mercator projection but it does not make sense since use_dms will be set to false. |
|
I will wait before the issuer gives some clarity. The PR now only contains a test. |
| def __call__(self, x, pos=None): | ||
| """ | ||
| Format the longitude, accounting for lon0 offset. | ||
| """ | ||
| # Adjust longitude value based on lon0 | ||
| adjusted_lon = x - self.lon0 | ||
| # Normalize to -180 to 180 range | ||
| adjusted_lon = ((adjusted_lon + 180) % 360) - 180 | ||
| print(x) | ||
| # Use the original formatter with the adjusted longitude | ||
| return super().__call__(adjusted_lon, pos) | ||
|
|
There was a problem hiding this comment.
Why was this method removed?
There was a problem hiding this comment.
I added this as the lon0 was not moving correctly, but I think the issue was in the formatter not being overriden with the @Poperty stuff -- potentially a change in the backend. I can confirm that it is shifting properly as the test is passing without this block, but not with this block. Also confirmed visually.
* don't set the formatter; handle through init * add test * rm lon0 logic * restore lazy loading * more restoring * more restoring * replace logic with rectilinear * update test * update tests * recover * revert most changes * reshuffle some code and rm dead code * fix and slight shuffle * update unittest * mv logic back * mv logic back * mv logic back * mv logic back * mv logic back * rm print * don't need this anymore * make error more helpful
* don't set the formatter; handle through init * add test * rm lon0 logic * restore lazy loading * more restoring * more restoring * replace logic with rectilinear * update test * update tests * recover * revert most changes * reshuffle some code and rm dead code * fix and slight shuffle * update unittest * mv logic back * mv logic back * mv logic back * mv logic back * mv logic back * rm print * don't need this anymore * make error more helpful
2 participants
Addresses #292 where DMS is not set for some axes that should have it set.