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 xarray shading #581

Merged
merged 7 commits into from
Sep 11, 2020
Merged

Add a test for xarray shading #581

merged 7 commits into from
Sep 11, 2020

Conversation

seisman
Copy link
Member

@seisman seisman commented Sep 4, 2020

Description of proposed changes

This PR adds a test checking if grid shading works for xarray.

It may work for GMT 6.1.0, but unfortunately, it doesn't work for GMT 6.1.1.

Tried to address #364 but failed.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

@seisman seisman marked this pull request as draft September 4, 2020 05:14
@seisman seisman added the maintenance Boring but important stuff for the core devs label Sep 4, 2020
@seisman
Copy link
Member Author

seisman commented Sep 4, 2020

Here are the generated images:

Using grid Using xarray GMT CLI (no shading)
image image image

The GMT CLI one is generated using the following command:

gmt grdimage @earth_relief_01d_g -R-180/180/-90/90 -B -JCyl_stere/6i -Cgeo -png test_grdimage_xarray_shading-noshading

It seems shading is not applied for xarray grids.

@weiji14
Copy link
Member

weiji14 commented Sep 4, 2020

Weird, can you try with shading="+d" as @liamtoney mentioned at #364 (comment)?

@seisman
Copy link
Member Author

seisman commented Sep 4, 2020

No changes at all.

@seisman
Copy link
Member Author

seisman commented Sep 4, 2020

In #364 (comment), I mentioned that shading works well for 6.1.0, but now no shading at all, and sometimes crashes.

@weiji14
Copy link
Member

weiji14 commented Sep 4, 2020

Possible regression in GMT? 🤦 Hold on, let me try this branch.

@seisman
Copy link
Member Author

seisman commented Sep 4, 2020

Sometimes I even get an image like this one:

image

@seisman
Copy link
Member Author

seisman commented Sep 4, 2020

Possible regression in GMT?

Yes, that's possible. There are some bigger changes in dealing with matrix grids in 6.1.1.

@weiji14
Copy link
Member

weiji14 commented Sep 4, 2020

Yes, I can confirm this locally. Also get an whitened image sometimes (compared to your darker one). Next thing I would try is with the GMT_IS_DUPLICATE/GMT_IS_REFERENCE settings.

@weiji14 weiji14 mentioned this pull request Sep 6, 2020
11 tasks
@seisman
Copy link
Member Author

seisman commented Sep 10, 2020

The test won't pass unless the upstream GMT fixes the bug. Maybe we can mark the test as "xfail" and merge it?

@weiji14
Copy link
Member

weiji14 commented Sep 10, 2020

The test won't pass unless the upstream GMT fixes the bug. Maybe we can mark the test as "xfail" and merge it?

Do we know what's wrong here (since 6.1.0 works)? I'm ok with using xfail, but please remove "Fixes #364." from the top post first.

@seisman
Copy link
Member Author

seisman commented Sep 10, 2020

Do we know what's wrong here (since 6.1.0 works)?

There are a lot of GMT API changes in GMT 6.1.1. These changes fix some crashes and issues, but unfortunately, break the grid shading. We may file another bug report to GMT later, and hopefully, it can be fixed again in GMT 6.2.0.

@weiji14
Copy link
Member

weiji14 commented Sep 11, 2020

Can we take this PR out of draft mode @seisman? It looks ready for review/approval, the Windows test failure is just that random one that occasionally breaks.

@seisman seisman marked this pull request as ready for review September 11, 2020 01:43
@seisman seisman requested a review from weiji14 September 11, 2020 01:43
@seisman seisman added this to the 0.2.x milestone Sep 11, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants