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

Final grdimage upgrade related to transparency #8241

Closed
wants to merge 24 commits into from

Conversation

PaulWessel
Copy link
Member

This branch replaces the deprecated branch #8191 which added some complications. This branch still passes 100% of the tests. I will now work on adding tests that highlight new capabilities (like variable transparency via square polygons) while making sure all tests still pass.

@PaulWessel PaulWessel added the enhancement Improving an existing feature label Dec 28, 2023
@PaulWessel PaulWessel added this to the 6.5.0 milestone Dec 28, 2023
@PaulWessel PaulWessel self-assigned this Dec 28, 2023
Copy link
Contributor

github-actions bot commented Dec 28, 2023

Summary of changed images

This is an auto-generated report of images that have changed on the DVC remote

Status Path
added test/baseline/grdimage/grdimage_Q_effects.ps
added test/baseline/grdimage/grid_transp.ps
added test/baseline/grdimage/image_vartrans.ps
added test/baseline/grdimage/transp_mix.ps
modified test/baseline/grdimage/

Image diff(s)

Added images

  • test/baseline/grdimage/grdimage_Q_effects.png

  • test/baseline/grdimage/grid_transp.png

  • test/baseline/grdimage/image_vartrans.png

  • test/baseline/grdimage/transp_mix.png

Modified images

Path Old New

Report last updated at commit 41df65d

@seisman
Copy link
Member

seisman commented Dec 28, 2023

Building fails on Linux:

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:46: error: ‘x’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                              ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:46: note: each undeclared identifier is reported only once for each function it appears in
/home/runner/work/gmt/gmt/src/grdimage.c:1313:72: error: ‘y’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                                                        ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:74: error: ‘dim’ undeclared (first use in this function); did you mean ‘idim’?
 1313 | #pragma omp parallel for private(srow,kk_s,k,x,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,Ctrl,H_s,H_i,image)
      |                                                                          ^~~

Did not test OpenMP so missed some new variables.
@PaulWessel
Copy link
Member Author

Oops, was not using OpenMP here so that was never exercised. Fixed now I hope - let me know. Removed x, added ix,iy as shared, and idim as private.

@seisman
Copy link
Member

seisman commented Dec 28, 2023

Still fails:

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:75: error: ‘y’ undeclared (first use in this function)
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)
      |                                                                           ^
/home/runner/work/gmt/gmt/src/grdimage.c:1313:75: note: each undeclared identifier is reported only once for each function it appears in
/home/runner/work/gmt/gmt/src/grdimage.c:1313:77: error: ‘dim’ undeclared (first use in this function); did you mean ‘idim’?
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,y,dim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)
      |                                                                             ^~~
      |                                                                             idim

@PaulWessel
Copy link
Member Author

Sorry, blindness. Now y is gone and dim is idim

@seisman
Copy link
Member

seisman commented Dec 28, 2023

/home/runner/work/gmt/gmt/src/grdimage.c: In function ‘grdimage_img_variable_transparency’:
/home/runner/work/gmt/gmt/src/grdimage.c:1313:33: error: ‘idim’ appears more than once in data clauses
 1313 | #pragma omp parallel for private(srow,kk_s,k,idim,scol,node_s,node_i,fill,idim) shared(GMT,Conf,iy,ix,Ctrl,H_s,H_i,image)

@PaulWessel
Copy link
Member Author

Amazingly stupid. Sorry about this @seisman but hopefully this nʻth iteration works.

@seisman
Copy link
Member

seisman commented Dec 28, 2023

Yes, it works, and please add #!/usr/bin/env bash to ./test/grdimage/image_vartrans.sh

@PaulWessel
Copy link
Member Author

Thanks, and done.

@joa-quim
Copy link
Member

This still crashes

gmt grdimage RandomGeoTIFFWithAlpha.tiff -png lixo

@joa-quim
Copy link
Member

Tried to fix the indexed images case with a transp value set by the nan_value by adding a I->alpha but didn't work. Debugged but couldn't find why.

@joa-quim
Copy link
Member

The trans_mix test and image_vartrans fail for me with corrupted PS. Here goes one (not finished PS).
transp_mix.ps.zip

@PaulWessel
Copy link
Member Author

Two issues I think.

  1. You have to run dvc pull to get the latest PS originals.
  2. As for the other branch (img-layout) I had forgotten to merge master into it.

With grdimage-upgrade branch selected and git pulled, and dvc pulled everything works just fine. No reason this should not work for you as well. Please test again.

@joa-quim
Copy link
Member

joa-quim commented Dec 30, 2023

You haven’t open the PS I attached. The problem is more serious and I don’t know how to debug it. Nor have time these next 3 days.

@PaulWessel
Copy link
Member Author

I did look at it, and was pushed, so I ran transp_mix.sh again and it crashed. Reason was I had not updated git etc as I mentioned so it complained that grdimage -Q+i was not available (it wasn't before). After git pull etc and rebuild it ran fine. So please run transp_mix.sh again manually (maybe bash -xv transp_mix.sh) and see if you still get any errors or message, and let me know. Hard to debug what does not crash here but it is clearly at the end of the square-transparency call.

@PaulWessel
Copy link
Member Author

Comparing your and my PS. While I have

PSL_font_encode 0 get 0 eq {ISOLatin1+_Encoding /Helvetica /Helvetica PSL_reencode PSL_font_encode 0 1 put} if
0 -114 M 142 F0
(180°) tc Z

...
you have

PSL_font_encode 0 get 0 eq {Standard+_Encoding /Helvetica /Helvetica PSL_reencode PSL_font_encode 0 1 put} if
0 -114 M 142 F0
(180�) tc Z

Not sure why you use Standard+ and how, and the degree symbol is an odd glyph?

You also have a lot of color setting duplicates, e.g.,

11.8 -12.2 1582.7 838.6 SS
{0.924 0.958 0.965 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
{0.924 0.926 0.936 C} FS
11.8 -12.2 968.5 732.3 SS

which I dont understand how can happen. I dont see that since they are the same color as before. But, it is possible this is some roundoff causing trouble when I check if next color is the same or not as the previous. I'll see how that is handled.

@PaulWessel
Copy link
Member Author

I use this to determine if the new and old color are the same:

#define PSL_same_rgb(a,b) (PSL_eq(a[0],b[0]) && PSL_eq(a[1],b[1]) && PSL_eq(a[2],b[2]) && PSL_eq(a[3],b[3])) /* If two colors are ~identical */

with

#define PSL_eq(a,b) (fabs((a)-(b)) < PSL_SMALL) /* If two color component are ~identical */

and PSL_SMALL = 1.0e-10.
Maybe that is too small on Windows. We use 10-9 in many places and certainly should be plenty here. Maybe you can try 10-9 and see if you get any repeat color lines.

@joa-quim
Copy link
Member

I was using latest git (nothing new comes out on a git pull)

It would be a disaster if different compilers gave different results on a comparison to < 1e-10. They all round off to same numbers, that's IEEE thing. A thing that my change it is the use of fast-math that should not be used to build the ref PSs.

Running the transp_mix.sh script directly and with debug mode shows:

j@dell-from-hell MINGW64 /c/v/build/test/grdimage/transp_mix
$ ./transp_mix.sh
PSL:C:\progs_cygw\GMTdev\gmt5\master\src\postscriptlight.c:6188: Error: Could not allocate memory [13881334300.86 Gb, 14904969211763476135 items of 1 bytes]
PSL:C:\progs_cygw\GMTdev\gmt5\master\src\postscriptlight.c:6188: Error: Could not allocate memory [8680354956.13 Gb, 9320460163558510294 items of 1 bytes]
./transp_mix.sh: line 34:  1186 Segmentation fault      gmt grdimage rgba.tif

@PaulWessel
Copy link
Member Author

Sorry, but I cannot determine what goes wrong on Windows. You should be able to place a stop point at line 607 and 618 and then go up and see why you get crazy sizes?

@joa-quim
Copy link
Member

joa-quim commented Dec 30, 2023

Shit happens in gmt grdimage rgba.tif -Rd -JQ9c

It's the OMP block in grdimage#L1313. I just learned that I can't debug inside it. The fill struct is invisible but when I let it go inside gmt_setfill I can see it has lots of garbage.

image

Commenting the OMP makes it work

@PaulWessel
Copy link
Member Author

Ah OpenMP. That makes sense since Xcode does not have it. K, will do the gcc build we use for the release then and I suspect I will figure that out quickly, Thanks!

@joa-quim
Copy link
Member

We should have checks to see if the OMP blocks actually speed up or not.

@PaulWessel
Copy link
Member Author

Built with OMP and got crash etc so will work on this tomorrow.

@PaulWessel
Copy link
Member Author

So learning a few things about OPenMP here. The initial issue was that even though I memset fill to 0 before the loops and use fill as a private variable for each thread, it clearly was not passed in to each thread as empty. So the PS had output in it as if it had pattern fill. Just junk.

I then placed the memset init inside the for loop over columns so it was set to NULL before we start filling in fill.rgb. Now we have no more image, pattern crap. But, still problems. The issue is at least partly the check in PSL if the new rgb differs from the last one sat. Well, what does that mean when there are 10 threads doing this and the PSL is doing this

else if (PSL_same_rgb (rgb, PSL->current.rgb[PSL_IS_FILL]))
	{ /* Skipped, fill already set */ }

This test fails because we implemented this to think that if we had just set the same rgb then skip it, but "just" does not work in parallel mode. So we end up with duplications in the PS:

{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS
{0.514 0.514 0.593 C} FS

And once in a while we get a junk character:

{0.53 0.514 0.593 C�} FS

Those are the things that crash gs, not the duplications.

Conclusion seems to be we cannot do OPM if we change color or transparency inside those loops. So probably no point with OPM in those cases. This is very different from the calculation OPM which does not depend on some recently set variable like in PSL.

Agree on reming OpenMP when PSL functions expecting linear memory is engaged, @joa-quim ?

@joa-quim
Copy link
Member

Sure. If it can’t be used.

@PaulWessel
Copy link
Member Author

Gonna try one more thing. We have no gmt_setcolor function (only set fill) which complicates this as I need to pass that fill structure that the f**s us. If that still fails then most of the loops with OPM are ok; it is only the new ones with fill.rgb that fails (hence most tests were not affected - just the new stuff). Will update you soon

@PaulWessel
Copy link
Member Author

Removed only the OPENMP loop that surrounds the plotting of squares since that is where the color and previous color enters. See how it works for you now.

@PaulWessel
Copy link
Member Author

One issue. Here is revised trains_mix.sh output via PDF (we had two identical plots so I replaced on with a constant 70% transparency):

tr

To make the fake transparency, which goes towards 1 in UR show that behavior I had to add +i to grdmix -Atransparency.nc+i, basically saying I am passing opacity (so it then converts it). But that does not make sense to me. We are passing transparency so perhaps the grdmix internals need to have a 1-x correction if +i is not given so that the image matches what we expect?

@PaulWessel
Copy link
Member Author

Updated some PS files (dvc pull) and all pass except two colorbar categorical plots due to the new #8246 PR. FYI, I will remove that as it messed with the placement of the label and causes two failures. I think the best plan is to

  1. Just implement the < and > label stuff for now but leave positioning and guessing of panel dimensions for now.
  2. After 6.5, spend the effort to determine the panel dimension in PostScript based on actually size of the labels. As it is, categorical colorers with AAA-BBB label cannot correctly set the panel dimensions and one must use the +c clearance trial-and-error. I'd rather make that modifier obsolete by correctly computing the PS dimensions, next year.

@joa-quim
Copy link
Member

joa-quim commented Dec 31, 2023

See how it works for you now.

$ ./transp_mix.sh
grdimage [ERROR]: Option -Q parsing failure.  Correct syntax:

  -Q<color>
      Specify <color> as one of:
       • <gray> or <red>/<green>/<blue>, all in range 0-255;
       • <cyan>/<magenta>/<yellow>/<black> in range 0-100%;
       • <hue>-<saturation>-<value> in ranges 0-360, 0-1, 0-1;
       • Any valid color name.
     For PDF fill transparency, append @<transparency> in the range 0-100% [0 = opaque].

let's finish this. I cannot be at the computer today and tomorrow.

This was in master. With the branch it still crashes.

@PaulWessel
Copy link
Member Author

I know you can't. And I cannot work at the moment but maybe a bit later.

@seisman
Copy link
Member

seisman commented Jan 7, 2024

As I understand it, the PR is already superseded by PR #8251. So closing.

@seisman seisman closed this Jan 7, 2024
@seisman seisman deleted the grdimage-upgrade branch January 7, 2024 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improving an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants