Skip to content

Conversation

@PaulWessel
Copy link
Member

@PaulWessel PaulWessel commented Sep 14, 2020

Description of proposed changes

grdimage has become very hard to debug due to ad hoc growth in capabilities. While it initially only dealt with plotting grids via CPTs it now can read images as well, and the grids may be remote or tiled. With intensity grids possibly to be derived from the tiled grids, the order of reading and calculations changed, making the flow much more variable.

This PR achieves several things:

  1. It splits the specific loops that convert a (projected or original) grid or image to a byte image into a series of functions, one for each scenario (grid vs image, intensity vs no intensity, grayscale vs color, colormasking vs no colormasking).
  2. Because there now are many functions with fewer tests inside the loops, more of the cases are now covered by OpenMP.
  3. By relying on the gmt_M_ijpgi macro the algorithms for finding the grid or image nodes are clean and straightforward. The gmt_M_ijpgi had a bug but did not have any affect before since col was always passed as zero.

The same tests that pass in master now pass in this branch. However, I have not yet tested this with OpenMP hence I added the WIP prefix. Also, I'd like @joa-quim to have a detailed look since both he and I made many changes to grdimage over the years so we need a more careful review given the importance of this specific module to GMT.

@seisman
Copy link
Member

seisman commented Sep 14, 2020

FYI, this PR fails on Linux:

../src/grdimage.c: In function ‘grdimage_img_c2s_with_intensity’:
../src/grdimage.c:924:33: error: ‘index’ is not a variable in clause ‘private’
 #pragma omp parallel for private(srow,byte,kk,k,scol,node,index,rgb) shared(GMT,Conf,Ctrl,image)
                                 ^
../src/grdimage.c: In function ‘grdimage_img_c2s_no_intensity’:
../src/grdimage.c:954:33: error: ‘index’ is not a variable in clause ‘private’
 #pragma omp parallel for private(srow,byte,kk,k,scol,node,index,rgb) shared(GMT,Conf,Ctrl,image)
                                 ^
../src/grdimage.c:945:91: warning: unused parameter ‘Ctrl’ [-Wunused-parameter]
 GMT_LOCAL void grdimage_img_c2s_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                                           ^~~~
../src/grdimage.c: In function ‘grdimage_img_color_no_intensity’:
../src/grdimage.c:978:33: error: ‘index’ is not a variable in clause ‘private’
 #pragma omp parallel for private(srow,byte,kk,k,scol,node,index,rgb) shared(GMT,Conf,Ctrl,image)
                                 ^
../src/grdimage.c:969:93: warning: unused parameter ‘Ctrl’ [-Wunused-parameter]
 GMT_LOCAL void grdimage_img_color_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                                             ^~~~
../src/grdimage.c: In function ‘grdimage_img_color_with_intensity’:
../src/grdimage.c:1002:33: error: ‘index’ is not a variable in clause ‘private’
 #pragma omp parallel for private(srow,byte,kk,k,scol,node,index,rgb) shared(GMT,Conf,Ctrl,image)

@PaulWessel PaulWessel changed the title WIP Refactor grdimage using functions Refactor grdimage using functions Sep 15, 2020
@PaulWessel
Copy link
Member Author

I built using gcc-mp and ran all tests (-j1) using the OpenMP enabled GMT. No failures, so I think this branch is good to merge now.

@seisman
Copy link
Member

seisman commented Sep 15, 2020

FYI, homebrew may be broken recently. That's why the macOS CI fails in all our CI jobs.

@seisman
Copy link
Member

seisman commented Sep 20, 2020

@PaulWessel Please merge master to keep this branch up-to-date.

@PaulWessel
Copy link
Member Author

Is there reasons for not approving this PR? Seems to pass all tests, no? Let me know if there are remaining issues with this PR on Windows @joa-quim or in the tests, @seisman. We still have the #4044 that I still think may not be completed - this is related to the subset of images.

@seisman
Copy link
Member

seisman commented Sep 20, 2020

I see some compilation warnings:

[113/266] Building C object src/CMakeFiles/gmtlib.dir/gmt_support.c.o
../../src/gmt_support.c:12822:79: warning: unused parameter 'flag' [-Wunused-parameter]
int gmt_getscale (struct GMT_CTRL *GMT, char option, char *text, unsigned int flag, struct GMT_MAP_SCALE *ms) {
                                                                              ^
1 warning generated.
[158/266] Building C object src/CMakeFiles/gmtlib.dir/grdimage.c.o
../../src/grdimage.c:659:92: warning: unused parameter 'Ctrl' [-Wunused-parameter]
GMT_LOCAL void grdimage_grd_gray_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                                           ^
../../src/grdimage.c:707:91: warning: unused parameter 'Ctrl' [-Wunused-parameter]
GMT_LOCAL void grdimage_grd_c2s_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                                          ^
../../src/grdimage.c:755:93: warning: unused parameter 'Ctrl' [-Wunused-parameter]
GMT_LOCAL void grdimage_grd_color_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                                            ^
../../src/grdimage.c:776:96: warning: unused parameter 'Ctrl' [-Wunused-parameter]
GMT_LOCAL void grdimage_grd_color_no_intensity_CM (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image, unsigned char *rgb_used) {
                                                                                               ^
../../src/grdimage.c:896:65: warning: unused parameter 'GMT' [-Wunused-parameter]
GMT_LOCAL void grdimage_img_gray_no_intensity (struct GMT_CTRL *GMT, struct GRDIMAGE_CTRL *Ctrl, struct GRDIMAGE_CONF *Conf, unsigned char *image) {
                                                                ^
../../src/grdimage.c:1040:24: warning: unused variable 'byte' [-Wunused-variable]
        uint64_t node, k, kk, byte, dim[GMT_DIM_SIZE] = {0, 0, 3, 0};
                              ^
../../src/grdimage.c:1046:53: warning: unused variable 'rgb' [-Wunused-variable]
        double dx, dy, x_side, y_side, x0 = 0.0, y0 = 0.0, rgb[4] = {0.0, 0.0, 0.0, 0.0};
                                                           ^

@PaulWessel
Copy link
Member Author

Thanks, I will leave the gmt_getscale warning there since this resulted from the work to enhance +c and it looks like that flag is not used - I need to understand this a bit more before I simply remove that parameters rather than marking it unused.

@PaulWessel PaulWessel merged commit a4690ca into master Sep 20, 2020
@PaulWessel PaulWessel deleted the grdimage-part2 branch September 20, 2020 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants