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

Add a test for plotting global grid without the redundant 360 longitude #3358

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

seisman
Copy link
Member

@seisman seisman commented Jul 25, 2024

Description of proposed changes

This PR adds a test for the bug reported in #3331. See #3331 (comment) for the comprehensive tests for when the bug can be triggered. This PR only test the central longitude=120 case. The baseline image is created by saving da3/da4 into a netCDF file and passing the netCDF files directly to Fig.grdimage.

The new test only passes with the upstream fix in GenericMappingTools/gmt#8554. So, the test is marked as xfail for GMT<=6.5.0.

The test passes in the "GMT Dev Tests" workflow: https://github.com/GenericMappingTools/pygmt/actions/runs/11138377051?pr=3358

Closes #3331.

@seisman seisman added bug Something isn't working upstream Bug or missing feature of upstream core GMT run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR labels Jul 25, 2024
@seisman seisman added this to the 0.13.0 milestone Jul 25, 2024
Copy link
Contributor

github-actions bot commented Jul 25, 2024

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
added pygmt/tests/baseline/test_grdimage_grid_no_redunant_360.png

Image diff(s)

Added images

  • test_grdimage_grid_no_redunant_360.png

Modified images

Path Old New

Report last updated at commit 86b6d7d

@seisman seisman marked this pull request as ready for review July 25, 2024 15:48
@seisman seisman added needs review This PR has higher priority and needs review. and removed run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR labels Jul 26, 2024
@weiji14 weiji14 added the run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR label Jul 29, 2024
@seisman

This comment was marked as outdated.

@seisman seisman removed the needs review This PR has higher priority and needs review. label Jul 29, 2024
@seisman seisman marked this pull request as draft July 29, 2024 02:42
@weiji14
Copy link
Member

weiji14 commented Jul 29, 2024

It's not a real global grid, which doesn't have the 360° longitude defined and also the north/south poles are missing. I'll open a separate PR to refine the xrgrid into a true global grid, so mark this PR "Draft" now.

Maybe best if we keep the original xrgrid that has data from 0-359° longitude (no redundant data at 360°), and a new grid from 0-360° longitude (redundant data at 360°). There isn't really a correct 'real' global grid here, we just need to know GMT/PyGMT can handle both.

@seisman
Copy link
Member Author

seisman commented Jul 29, 2024

It's not a real global grid, which doesn't have the 360° longitude defined and also the north/south poles are missing. I'll open a separate PR to refine the xrgrid into a true global grid, so mark this PR "Draft" now.

Maybe best if we keep the original xrgrid that has data from 0-359° longitude (no redundant data at 360°), and a new grid from 0-360° longitude (redundant data at 360°). There isn't really a correct 'real' global grid here, we just need to know GMT/PyGMT can handle both.

Actually the original issue is not due to the missing 360 longitude. For any grids that is not exactly 0-360, the same issue occurs for central longitude < 180 and for specific projections. If interested, you can play with the script in #3331 (comment).

@seisman seisman added this to the 0.14.0 milestone Oct 2, 2024
@seisman seisman marked this pull request as ready for review October 2, 2024 04:37
@seisman seisman changed the title Add a test for checking xr.DataArray grid with Hammer projection Add a test for plotting global grid without the redundant 360 longitude Oct 2, 2024
@seisman seisman added maintenance Boring but important stuff for the core devs and removed bug Something isn't working labels Oct 2, 2024
@seisman seisman added the needs review This PR has higher priority and needs review. label Oct 2, 2024
@seisman seisman requested a review from a team October 5, 2024 15:32
@seisman seisman marked this pull request as draft October 6, 2024 01:45
@seisman seisman removed the needs review This PR has higher priority and needs review. label Oct 6, 2024
@seisman seisman marked this pull request as ready for review October 6, 2024 10:28
@seisman seisman added needs review This PR has higher priority and needs review. final review call This PR requires final review and approval from a second reviewer and removed needs review This PR has higher priority and needs review. labels Oct 6, 2024
pygmt/tests/test_grdimage.py Outdated Show resolved Hide resolved
pygmt/tests/test_grdimage.py Outdated Show resolved Hide resolved
pygmt/tests/test_grdimage.py Outdated Show resolved Hide resolved
Co-authored-by: Yvonne Fröhlich <[email protected]>
@seisman seisman merged commit 1080ade into main Oct 7, 2024
20 of 21 checks passed
@seisman seisman deleted the test/grdimage branch October 7, 2024 12:23
@seisman seisman removed final review call This PR requires final review and approval from a second reviewer run/test-gmt-dev Trigger the GMT Dev Tests workflow in PR labels Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance Boring but important stuff for the core devs upstream Bug or missing feature of upstream core GMT
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grdimage does not work correctly when the redundant 360 E latitude is not included
3 participants