-
Notifications
You must be signed in to change notification settings - Fork 401
Several problems in grdgradient #4328
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grdgradient computes derivatives and requires at least a one row/col padding for the algorithm to work: We do the finite differencing on a bin with 4 nodes and store the result in the top left node which is not used in further calcuations. At the end we restore the padding so the output is correctly aligned. For files (automatically having a pad of 2) this works fine.
seisman
approved these changes
Oct 14, 2020
This was referenced Oct 15, 2020
seisman
added a commit
to GenericMappingTools/pygmt
that referenced
this pull request
Oct 15, 2020
The xarray shading issue in #364 was fixed by upstream GMT in GenericMappingTools/gmt#4328. This PR updates the pytest xfail condition so that the xarray shading test is expected to xfail only for GMT<=6.1.1.
This was referenced Oct 15, 2020
Member
|
@PaulWessel This PR works well for most cases but crashes for a constant intensity for both CLI and PyGMT. Here is the CLI crash: |
Member
Author
|
All those struct GMT_GRID_HEADER *H_i = Conf->Intens->header; needs to be: struct GMT_GRID_HEADER *H_i = (Conf->Intens) ? Conf->Intens->header : NULL; Heading out if you want to do this. |
Member
Author
|
I'm back, unless @seisman started on this I can do it. |
Member
|
I'm not working on it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
grdgradient computes derivatives and requires at least a one row/col padding for the algorithm to work: We do the finite differencing on a cell defined by 4 nodes and store the result in the top left node which is not used in further calculations. At the end, we restore the padding so the output grid is correctly aligned. For files (automatically having a pad of 2) this works fine.
When grdgradient is called via GMT_Call_Module with a memory grid that has no padding (#3408) we detect that case and call
new_grid = gmt_set_outgrid (GMT, Ctrl->In.file, separate, 1, Surf, &Out); /* true if input is a read-only array */This function returns true in this case and allocates a new grid (Out) that (given the arg = 1) will have the required minimum of one row/col padding. We then place computed gradients into
Out->data[index]so that Surf (read-only) is not touched. But wait a minute! The loop we set up is using the Surf->header to get the index into the Surf array, but in this case Surf has no padding. Note we always do this:so p[1] and p[3] are always negative relative node changes. Then in the loop we do this
for (n = 0, bad = false; !bad && n < 4; n++) if (gmt_M_is_fnan (Surf->data[ij+p[n]])) bad = true;Since
Surf->datahas no pad and ij starts at 0, we end up with negative array indices. Not sure why it is not crashing but it clearly points to nowhere. This explains part of the problem in #3408 but regardless this is a serious bug.Another issues is -S. With -S we create another output grid Slope by duplicating Surf. We then use the same index as Out to assign output. But this will be wrong if Surf has no pad. Now I allocate Slope using the same padding as the output grid.
Finally, ensuring that the grid has at least one row/col is useless since gmt_set_outgrid may in fact need to call gmt_grd_BC_set which requires two rows/cols. So now we call it with two.
For clarity, this PR replaces "Surf" with "In", and "Out" with "Grid" and uses Grid in all those places since it is this array we get values from and then we always place the result in the top-left node of the cells. At the end we reinstate the pad. Doing this and running all the test gave no errors, and it also fixes #3408.