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

Bump the required minimum GMT version to 6.1.0 #507

Merged
merged 29 commits into from
Jul 11, 2020
Merged

Conversation

seisman
Copy link
Member

@seisman seisman commented Jul 8, 2020

Description of proposed changes

  • Bump the required minimum GMT version to 6.1.0
  • Fix some failing tests (only numbers, not baseline images) due to the change
    of default earth relief grid registration
  • Remove compatibility codes for GMT 6.0
  • Use pytest.mark.xfail to ignore some failures due to outdated baseline images or unknown failure reasons
  • The Windows builds are disabled due to crashes
  • The documentation building are disabled due to crashes on macos

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
Copy link
Member Author

seisman commented Jul 8, 2020

@weiji14 It's a little complicated here. In this PR, I bump the required minimum GMT version to 6.1.0. Apparently, there are tens of failures due to the change of default earth relief grid registration between GMT 6.0 and 6.1.

To fix these failing tests, I need to update the load_earth_relief() function:

  1. Allow users to specify the grid registration of earth relief data
  2. Support all resolutions from 01d to 01s
  3. Allow users to specify a region, which is especially important for plotting high-resolution relief in small regions

Such changes may cause conflicts with your PRs #494 and #500, and this PR may be too large to fix so many things.

Maybe I should do only one thing in this PR (i.e., bumping the GMT version) and merge it although there are so many failing tests, then open more PRs to fix these failures one by one?

Do you have a better idea to make the 6.1.0 update simpler?

@weiji14
Copy link
Member

weiji14 commented Jul 8, 2020

Yeah, I agree with what you say, the load_earth_relief function needs to be redesigned first as in #489, but there's a bit of a chicken and egg situation since we need this 6.1.0 PR to do so. Ideally we would have the Github Actions CI doing a matrix build on GMT 6.0 and 6.1 first, but that's also a bit of a chore. This is one of the reasons I opened up #505 so as to reduce our dependency on load_earth_relief for our tests. Will need to have a longer think about this.

P.S. Don't mind the conflicts with those PRs. I would say that #500 actually supersedes #494 (will close it after #500 is merged), and it shouldn't be too hard to update them since they're quite small changes.

@weiji14
Copy link
Member

weiji14 commented Jul 9, 2020

Ok, after some thought, this is the master plan I've come up with:

@seisman
Copy link
Member Author

seisman commented Jul 9, 2020

We're bumping to GMT 6.1.0. I don't understand why we need to test both 6.0 and 6.1 now and remove GMT 6.0 backward compatibility later. We can make any changes to make PyGMT work with 6.1, even though the changes break 6.0.

@weiji14
Copy link
Member

weiji14 commented Jul 9, 2020

In that case, let's do 3 - 1 - 2. Just break things now and fix them as we go along (similar to the Windows fix before). I just don't have time today (paper response deadline) and didn't want the tests to be broken for so long.

@seisman
Copy link
Member Author

seisman commented Jul 9, 2020

Azure Pipelines have some technical issues which fail all our CI jobs. We need to wait for its recovery.

@weiji14
Copy link
Member

weiji14 commented Jul 9, 2020

Is there a timeline on when Azure Pipelines would be fixed? The SciPy sprint is coming up, and if that doesn't get resolved, we might need to fast track #475. But keep the Travis CI builds since they still run (and we need it to do the PyPI releases).

@seisman
Copy link
Member Author

seisman commented Jul 9, 2020

@vercel vercel bot temporarily deployed to Preview July 10, 2020 05:26 Inactive
@vercel vercel bot temporarily deployed to Preview July 10, 2020 05:28 Inactive
@seisman
Copy link
Member Author

seisman commented Jul 10, 2020

@weiji14 I still think we should bump to 6.1.0 ASAP, and use the pytest.mark.xfail trick to "pass" the failing tests. It's not worth the effort making PRs compatible with both 6.0 and 6.1, and then remove the codes for 6.0 in just a few days.

Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with approving this PR and bumping to GMT 6.1.0 straightaway, but even if we do that, we still need to do #476 before we can update the baseline images. So go for it.

@seisman
Copy link
Member Author

seisman commented Jul 10, 2020

@weiji14 FYI, Azure Pipelines is back.

@seisman seisman marked this pull request as ready for review July 11, 2020 08:13
@seisman seisman merged commit b74bc3e into master Jul 11, 2020
@seisman seisman deleted the bump-gmt-6.1.0 branch July 11, 2020 08:14
@seisman seisman added the maintenance Boring but important stuff for the core devs label Jul 11, 2020
@seisman seisman added this to the 0.2.x milestone Jul 11, 2020
@seisman
Copy link
Member Author

seisman commented Jul 11, 2020

@weiji14 Welcome back. I merged three PRs (#512, #513 and #507) last night to make it work with GMT 6.1.0. Although these PRs were already merged, it would be better if you still give them a review and see if I made any mistakes.

@weiji14
Copy link
Member

weiji14 commented Jul 11, 2020

Cool, thanks for handling all of that! I've taken a quick look across all of them already and they seem fine, but I'll want to focus on the pixel/gridline registration issue today.

@seisman
Copy link
Member Author

seisman commented Jul 11, 2020

Yes, it has the highest priority.

@seisman seisman mentioned this pull request Jul 12, 2020
5 tasks
@weiji14 weiji14 mentioned this pull request Jul 16, 2020
5 tasks
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