Re: GSoC 16: Sample implementation for ispolycw function

[John Swensen]

On Mar 23, 2016, at 1:50 PM, amr mohamed <address@hidden> wrote:

Subject: Re: GSoC 16: Sample implementation for ispolycw function
From: address@hidden
Date: Wed, 23 Mar 2016 10:56:25 -0700
CC: address@hidden; address@hidden; address@hidden
To: address@hidden

On Mar 23, 2016, at 10:28 AM, amr mohamed <address@hidden> 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

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 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 .
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.