#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 by , 14 years ago
Cc: | added |
---|---|
Owner: | changed from | to
comment:2 by , 14 years ago
Component: | GDAL_Raster → Algorithms |
---|
comment:3 by , 14 years ago
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 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Committed to Trunk (18995) and 1.7 (18996)
follow-up: 6 comment:5 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
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"
follow-up: 10 comment:8 by , 14 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 by , 14 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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 by , 14 years ago
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
Gaige,
I red-facedly concur with your analysis. Please go ahead and apply these changes in trunk and 1.7 branch.