octave-maintainers
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: GSoC 16: Sample implementation for ispolycw function


From: PhilipNienhuis
Subject: Re: GSoC 16: Sample implementation for ispolycw function
Date: Sun, 17 Apr 2016 10:42:02 -0700 (PDT)

John Swensen-3 wrote
>> On Mar 23, 2016, at 1:50 PM, amr mohamed <

> amr_mohamed@

> > wrote:
>> 
>> Subject: Re: GSoC 16: Sample implementation for ispolycw function
>> From: 

> jpswensen@

>  <mailto:

> jpswensen@

> >
>> Date: Wed, 23 Mar 2016 10:56:25 -0700
>> CC: 

> pr.nienhuis@

>  <mailto:

> pr.nienhuis@

> >; 

> nirkrakauer@

>  <mailto:

> nirkrakauer@

> >; 

> octave-maintainers@

>  <mailto:

> octave-maintainers@

> >
>> To: 

> amr_mohamed@

>  <mailto:

> amr_mohamed@

> >
>> 
>> 
>> On Mar 23, 2016, at 10:28 AM, amr mohamed <

> amr_mohamed@

>  <mailto:

> amr_mohamed@

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

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]