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

Special attention when tile grids are passed to grdimage #3799

Merged
merged 1 commit into from
Aug 1, 2020

Conversation

PaulWessel
Copy link
Member

This addresses issue #3694. The problem turned out to be this:

grdimage usually reads in a grid in two steps:

  1. Read in the grid header only to determine the grid region
  2. After calling gmt_map_setup, which may adjust -R for global grids, we read the grid, this time passing the adjusted wesn for the map.

So, in the case that caused this issue, we may use -Rg and -JN120/20c, but after gmt_map_setup, the R.wesn is no longer 0/30 but -60/300. So the grid is read in correctly relative to the desired region.

However, when we read a global tiled region there is a difference: Because the grid is assembled by grdblend, the first call (to get the header) needs to do the whole thing since we are not going to assemble the grid twice just to be the region. Thus, the tiled region in this case remains 0/360 since there is no second read after projection is set. Nevertheless, none of this caused a problem until we also selected auto-shading. Now, the 0/360 grid is passed to grdgradient with the actual region and two grids are out of sync.

There may be more than one solution to this. Since this case is unique to grdimage the following solution was implemented there: Since we know we read a tiled_grid up front in this specific case (tiled grid + auto-shading) we simply pass the 0/360 region to grdgradient rather than the adjusted region. With that change, the case works and all tests work fine.

This addresses issue #3694.  The problem turned out to be this:

grdimage usually reads in a grid in two steps:
@PaulWessel PaulWessel added bug Something isn't working backport 6.1 Backport this PR to 6.1 branch labels Aug 1, 2020
@PaulWessel PaulWessel requested a review from seisman August 1, 2020 05:10
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

Great job!

@PaulWessel PaulWessel merged commit a336215 into master Aug 1, 2020
@PaulWessel PaulWessel deleted the tile-region-stability branch August 1, 2020 07:37
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2020

The backport to 6.1 failed:

The process 'git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-6.1 6.1
# Navigate to the new working tree
cd .worktrees/backport-6.1
# Create a new branch
git switch --create backport-3799-to-6.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick a336215a12b9d879f6cf097b1bd96f54706ab658
# Push it to GitHub
git push --set-upstream origin backport-3799-to-6.1
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-6.1

Then, create a pull request where the base branch is 6.1 and the compare/head branch is backport-3799-to-6.1.

@PaulWessel
Copy link
Member Author

Maybe have a look at this tomorrow, @seisman

@seisman
Copy link
Member

seisman commented Aug 1, 2020

There is a conflict in grdimage.c when cherry-picking this PR:

++<<<<<<< HEAD
 +              /* Prepare the grdgradient arguments using selected -A -N and the data region in effect */
 +              sprintf (cmd, "-G%s -A%s -N%s+a%s -R%.16g/%.16g/%.16g/%.16g --GMT_HISTORY=false ",
 +                      int_grd, Ctrl->I.azimuth, Ctrl->I.method, Ctrl->I.ambient, wesn[XLO], wesn[XHI], wesn[YLO], wesn[YHI]);
 +              if (got_data_grid)      /* Use the virtual file we made earlier */
++=======
+               /* Prepare the grdgradient arguments using selected -A -N and the data region in effect.  If we read in a tiled grid
+                * then it was made with 0/360 so we must use that region in grdgradient.  For non-tiles, we read the actual grid
+                * AFTER calling gmt_mapsetup which changes the region.  The tiled region remains unchanged because it is read in
+                * all at once as it is being assembled.  This fixes https://github.com/GenericMappingTools/gmt/issues/3694 */
+               sprintf (cmd, "-G%s -A%s -N%s+a%s -R%.16g/%.16g/%.16g/%.16g --GMT_HISTORY=readonly ",
+                       int_grd, Ctrl->I.azimuth, Ctrl->I.method, Ctrl->I.ambient, region[XLO], region[XHI], region[YLO], region[YHI]);
+               if (got_data_tiles)     /* Use the virtual file we made earlier */
++>>>>>>> a336215a12... Special attention when tile grids are passed to grdimage (#3799)

I believe I should accept the changes in the 2nd block, but change --GMT_HISTORY=readonly to --GMT_HISTORY=false (we didn't backport the GMT_HISTORY changes of #3687 to 6.1 branch).

seisman pushed a commit that referenced this pull request Aug 1, 2020
PaulWessel added a commit that referenced this pull request Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 6.1 Backport this PR to 6.1 branch bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants