Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#3447 closed defect (fixed)

GDALSuggestedWarpOutput2 does more work than necessary

Reported by: gaige Owned by: gaige
Priority: normal Milestone: 1.7.2
Component: Algorithms Version: 1.7.1
Severity: normal Keywords:
Cc: warmerdam

Description

The edge sample transformation test fails 100% of the time due to multiple errors. Fortunately, this has little effect on the user perception, since the grid test works just fine afterwards, resulting in a slightly longer process to check the bounds, but not creating a critical problem.

Among the problems were:

  • dfRatio wasn't recalculated, so all points were tested against points outside of the box
  • the test of the reverted points actually tested the original projected points instead
  • the test of the reverted points only tested the X coordinate (twice) instead of both the X and Y coordinates
==== //gdaltransformer.cpp#13 - gdaltransformer.cpp ====
***************
*** 428,433 ****
--- 428,437 ----
                  if( !abSuccess[i] )
                      nFailedCount++;
                  
+ 				dfRatio = 0.0 + floor(i/4)*(1.0/N_STEPS);
+ 				if (dfRatio>0.99)
+ 					dfRatio = 1.0;
+ 					
                  double dfExpectedX, dfExpectedY;
                  if ((i % 4) == 0)
                  {
***************
*** 450,457 ****
                      dfExpectedY   = dfRatio * nInYSize;
                  }
                  
!                 if (fabs(adfX[i] - dfExpectedX) > nInXSize / N_STEPS ||
!                     fabs(adfX[i] - dfExpectedX) > nInYSize / N_STEPS)
                      nFailedCount ++;
              }
          }
--- 454,461 ----
                      dfExpectedY   = dfRatio * nInYSize;
                  }
                  
!                 if (fabs(adfXRevert[i] - dfExpectedX) > nInXSize / N_STEPS ||
!                     fabs(adfYRevert[i] - dfExpectedY) > nInYSize / N_STEPS)
                      nFailedCount ++;
              }
          }

Change History (10)

comment:1 Changed 10 years ago by warmerdam

Cc: warmerdam added
Owner: changed from warmerdam to gaige

Gaige,

I red-facedly concur with your analysis. Please go ahead and apply these changes in trunk and 1.7 branch.

comment:2 Changed 10 years ago by warmerdam

Component: GDAL_RasterAlgorithms

comment:3 Changed 10 years ago by Even Rouault

Ahem, *I* ref-facedly concur. This is code I modified substantially for ticket #2305 (http://trac.osgeo.org/gdal/changeset/18035) during 1.7 developement. I remember it was quite tricky to come with, and as Gaige found, the code just after in the algorithm fallbacks with computations on the full grid, which hides the fact that this code just before was buggy. Your changes look right Gaige.

comment:4 Changed 10 years ago by gaige

Resolution: fixed
Status: newclosed

Committed to Trunk (18995) and 1.7 (18996)

comment:5 Changed 10 years ago by Even Rouault

Thanks !

Minor notes : in your commit messages, in addition to the ticket number, it would be good to add a short description of what the fix is. This makes the life of people writing the ChangeLog? easier ;-)

Ah, and a Trac tip, when you indicate a revision number, indicate the r before the number, such as r18995 so Trac automagically makes a hyperlink to the changeset.

comment:6 in reply to:  5 Changed 10 years ago by gaige

Replying to rouault:

Thanks !

Minor notes : in your commit messages, in addition to the ticket number, it would be good to add a short description of what the fix is. This makes the life of people writing the ChangeLog? easier ;-)

Ah, and a Trac tip, when you indicate a revision number, indicate the r before the number, such as r18995 so Trac automagically makes a hyperlink to the changeset.

Thanks. I looked for some examples and some specifics in the RFCs and didn't see them (maybe I looked in the wrong place), but I didn't find anything. Thanks and I'll make sure to do that in the future.

comment:7 Changed 10 years ago by Even Rouault

Yes, everything is not written. I've updated RFC3 to reflect my above advice. I think "Use meaningful descriptions for SVN commit log entries." in SVN Commit Practices paragraph matches what I meant with "add a short description of what the fix is"

comment:8 Changed 10 years ago by Even Rouault

Resolution: fixed
Status: closedreopened

Win32 doesn't like floor()...

gdaltransformer.cpp(431) : error C2668: 'floor' : ambiguous call to overloaded function
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(559): could be 'long double floor(long double)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(511): or 'float floor(float)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(137): or 'double floor(double)'
        while trying to match the argument list '(int)'

I take care of fixing this

comment:9 Changed 10 years ago by Even Rouault

Resolution: fixed
Status: reopenedclosed
r19003 /trunk/gdal/alg/gdaltransformer.cpp: Compilation fix for r18995 on Windows platform (#3447)

r19004 /branches/1.7/gdal/alg/gdaltransformer.cpp: Compilation fix for r18996 on Windows platform (#3447)

comment:10 in reply to:  8 Changed 10 years ago by gaige

Replying to rouault:

Win32 doesn't like floor()...

gdaltransformer.cpp(431) : error C2668: 'floor' : ambiguous call to overloaded function
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(559): could be 'long double floor(long double)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(511): or 'float floor(float)'
        C:\Program Files (x86)\Microsoft Visual Studio 8\VC\INCLUDE\math.h(137): or 'double floor(double)'
        while trying to match the argument list '(int)'

I take care of fixing this

Thanks for that. I need to figure out how the buildfarm works as I don't have a win32 development system to test against.

Next time, it'll be better... -Gaige

Note: See TracTickets for help on using tickets.