Re: GSoC 16: Sample implementation for ispolycw function

[Juan Pablo Carbajal]

On Mon, Apr 18, 2016 at 5:17 PM, John Swensen <address@hidden> wrote:
>
>> 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.
>
>
>
>
Geometry is a fork of matgeom, a m-file only geometry library. I am
happy if some of the cpu heavy routines are ported to c++ and can help
reviewing that code.