Re: GSoC 16: Sample implementation for ispolycw function

[John Swensen]

> On Apr 17, 2016, at 10:42 AM, PhilipNienhuis <address@hidden> wrote:
> 
> John Swensen-3 wrote
>>> On Mar 23, 2016, at 1:50 PM, amr mohamed &lt;
> 
>> amr_mohamed@
> 
>> &gt; wrote:
>>> 
>>> Subject: Re: GSoC 16: Sample implementation for ispolycw function
>>> From: 
> 
>> jpswensen@
> 
>> &lt;mailto:
> 
>> jpswensen@
> 
>> &gt;
>>> Date: Wed, 23 Mar 2016 10:56:25 -0700
>>> CC: 
> 
>> pr.nienhuis@
> 
>> &lt;mailto:
> 
>> pr.nienhuis@
> 
>> &gt;; 
> 
>> nirkrakauer@
> 
>> &lt;mailto:
> 
>> nirkrakauer@
> 
>> &gt;; 
> 
>> octave-maintainers@
> 
>> &lt;mailto:
> 
>> octave-maintainers@
> 
>> &gt;
>>> To: 
> 
>> amr_mohamed@
> 
>> &lt;mailto:
> 
>> amr_mohamed@
> 
>> &gt;
>>> 
>>> 
>>> On Mar 23, 2016, at 10:28 AM, amr mohamed &lt;
> 
>> amr_mohamed@
> 
>> &lt;mailto:
> 
>> amr_mohamed@
> 
>> &gt;> wrote:
>>> 
>>> Dear all,
>>> I have made a sample cc file for the ispolycw function using
>>> Boost/Geometry library.
>>> I have tested it and it is working well.
>>> Could you please review it so that i can finalize it? 
>>> https://ideone.com/w6WFzB &lt;https://ideone.com/w6WFzB&gt;
>>> 
>>> Regards, 
>>> Amr Mohamed
>>> 
>>> This looks like a good start of getting at least one function implemented
>>> using the Octave C++ API and including Boost::Geometry. 
>>> 
>>> Just two quick comments:
>>> 
>>> 1) You might also want to work on making this handle self-intersections
>>> in the same way that the Matlab documentation indicates they handle
>>> self-intersections
>>> 
>>> 2) To show that testing works (and correctly), you can include tests
>>> written in the Octave language as comments directly in the C++ code.
>>> See http://hg.octave.org/octave/file/tip/libinterp/dldfcn/chol.cc
>>> &lt;http://hg.octave.org/octave/file/tip/libinterp/dldfcn/chol.cc&gt;
>>> where there is the following example that checks whether the inverse from
>>> the Cholesky decomposition gives the same result as the standard matrix
>>> inverse computation.
>>> /*
>>> %!shared A, Ainv
>>> %! A = [2,0.2;0.2,1];
>>> %! Ainv = inv (A);
>>> %!test
>>> %! Ainv1 = cholinv (A);
>>> %! assert (norm (Ainv-Ainv1), 0, 1e-10);
>>> %!testif HAVE_CHOLMOD
>>> %! Ainv2 = inv (sparse (A));
>>> %! assert (norm (Ainv-Ainv2), 0, 1e-10);
>>> %!testif HAVE_CHOLMOD
>>> %! Ainv3 = cholinv (sparse (A));
>>> %! assert (norm (Ainv-Ainv3), 0, 1e-10);
>>> */
>>> 
>>> If you could include tests like this for at least one clockwise and one
>>> counterclockwise polygon, that would help show it works and allow there
>>> to be regression testing going forward in the case that something changes
>>> in Boost::Geometry or a bugfix causes a different error.
>>> 
>>> Let me know if you have any questions.
>>> 
>>> John S.
>>> 
>>> Dear Mr John,
>>> 
>>> I have included some test cases to the cc file as mentioned
>>> https://ideone.com/oAiVFP &lt;https://ideone.com/oAiVFP&gt; .
>>> I haven't handled the self-intersecting polygons case yet.
>>> 
>>> I have two questions:
>>> 1)Should i recommend a library for each function in the proposal? and if
>>> so , how can i choose between two libraries if both can be used to
>>> implement the function?
>>> 
>>> 2)Should i mention that i have made a sample for the ispolycw function ?
>>> and will it be better if a create a bitbucket repository instead of
>>> sharing my code through links?
>>> 
>>> Regards, 
>>> Amr Mohamed
>> 
>> 
>> 1) I would think that we would only want to include one library dependency
>> to implement all the missing polygon functionality. Otherwise, you would
>> have to spend a lot of effort learning two different libraries and how to
>> convert between their vertex representations.
>> 
>> 2) I think the best way to contribute code is to make sure it is complete.
>> For example don’t include things like “TODO: Check if arguments are valid”
>> when it is a pretty easy task to go ahead and perform those checks. Once
>> you think it is ready, I would submit a Bug Report marked as a Feature
>> Request to the GNU Octave Savannah project and then attach the file to the
>> bug report.
>> 
>> Two other comments:
>> I would definitely mention in your proposal that you are at the point when
>> you can compile external functions against the Octave libraries and the
>> Boost libraries and implemented one of the more simple polygon functions.
>> 
>> Also, you should look more carefully at how the tests were written in the
>> link I gave you. You should follow the pattern they used to include
>> “assert” statements that will let the automated test system know whether
>> the tests succeeded or failed.
>> 
>> John S.
> 
> PMFJI (very) late. 
> 
> We seem to overlook functions already present in other OF packages.
> 
> The geometry package contains polygonArea.m that returns the area of a
> polygon, or a set of polygons, which is negative for clockwise and positive
> for counterclockwise polygons.
> So we could have a very simple function ispolycw.m looking along the lines
> of:
> 
> function cw = ispolycw (<Nx2_POINTS>)
> 
> cw = polygonArea (<Nx2_POINTS>) < 0;
> 
> endfunction
> 
> that has no external dependencies whatsoever (apart from maybe an OF
> package).
> 
> Now, CPU performance may be a point. But for the time being, a function
> ispolycw.m w/o dependencies (the latter an aim of me) is in close reach.
> And I think it would fit nicely and naturally in the geometry package. (Yeah
> I'm stirring up again, sorry for that)
> 
> Philip
> 
> 
> 
> 
> --
> View this message in context: 
> http://octave.1599824.n4.nabble.com/GSoC-16-Sample-implementation-for-ispolycw-function-tp4675763p4676344.html
> Sent from the Octave - Maintainers mailing list archive at Nabble.com.
> 

I think that doing a quick performance check on the existing polygon 
functionality in Octave compared to the algorithms in Boost::Geometry and 
Boost::Polygon would be of merit. I think that one of the websites that did a 
comparison test between some other libraries gave the sets of points they used 
for their benchmarks. I know from interacting a bit on the mailing list that 
Boost::Geometry is very actively maintained by funded contributors from 
companies that do mapping and GIS. This alone makes it a somewhat attracted 
alternative. You are right, however, that it adds another dependency to the 
package(s) of geometry and mapping.

John S.