patchGNU Octave - Patches: patch #9494, Patches for polygon2patch &...

 
 

patch #9494: Patches for polygon2patch & joinPolygns

Submitted by:  Philip Nienhuis <philipnienhuis>
Submitted on:  Sun 19 Nov 2017 07:32:45 PM UTC  
 
Category:  Forge : other Priority:  5 - Normal
Status:  Ready For Test Privacy:  Public
Assigned to:  Juan Pablo Carbajal <juanpi> Open/Closed:  Open

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Sun 03 Dec 2017 08:38:35 PM UTC, comment #8:

@JuanPi:
Attached are patches for

  • polyjoin (rewritten, I've copied copyright strings + tests from the one in funcaliases). polyjoin now accepts * returns input vector in any orientation + matrices

and

  • polygon2patch (new patch, w/o CRLF issues)

I hope next weekend I can upload some more.

(file #42562, file #42563)

Philip Nienhuis <philipnienhuis>
Project Member
Sun 26 Nov 2017 06:39:24 PM UTC, comment #7:

All right.
It may take a little time.

When do you think you can make a new release?

I think I indeed missed all t\of this year 's GSOC mails, I wasn't in the circuit so to say. IIRC I only catched that re-implementing clipper() as an .oct file didn't pay out in terms of performance.

Philip Nienhuis <philipnienhuis>
Project Member
Sun 26 Nov 2017 04:37:48 AM UTC, comment #6:

Hi,

Yes definitely is worth merging.

I tihnk you missed all the mails I sent during the last GSoC, you wouldn't be so surprised now :D.

The criteria was: keep matgeom naming convention to avoid multiple changes (matGeom is adding some of these functions). Put geometric functions in the geometry package and not in mapping.

ispolycw:
The geometry function isPolygonCW[1] has both methods, either geometry or clipper. Seems the studnet didn't finish the job in the funcAliases folder, they shouldn't implement functionality, just wrap. The idea is that it has a default argument (either geometry or clipper) and calls the geometry function.

If you have the time, please go ahead with the merging.

[1]: https://sourceforge.net/p/octave/geometry/ci/2be7484d72c9cf0c021c16a5a9852e23678792dd/tree/inst/polygons2d/isPolygonCCW.m

Juan Pablo Carbajal <juanpi>
Project MemberIn charge of this item.
Fri 24 Nov 2017 05:07:20 PM UTC, comment #5:

@JuanPi:

A few things.

1. After the lenghty discussion about where the polygon functions should live we decided on the geometry package.
What I didn't expect was that you would rename them ("polygonClip" etc.), where I figured the Matlab-comaptible names & options would be used straight away.
Well, fine :-)
So then I thought the intention was to keep your naming scheme in geometry but put Matlab-compatible wrappers in the mapping pkg that would call the actual workhorses in the geometry package.
Now it turns out that the Matlab-compatible wrappers are hosted in the geometry package as well.
Well, fine too :-)

2. Comparing your and mine tree
Only polyjoin and ispolycw overlap.

polyjoin.m:
- yours only accepts column vectors; got some tests
- mine accepts (& returns) row vectors or column vectors, similar to input dimension, and also Nx2 or N3 matrices; no tests yet

ispolycw.m:
- yours only accepts column vectors, is based on polygonArea
- mine accepts row & column vectors + Nx2 or Nx3 arrays; is based on clipper

polybool.m:
- yours: N/A
- mine: implements all of matlabs options and is based on clipper (in geometry). Needs a demo

- polysplit, poly2cw, ispolyccw:
- yours: present, only accept column vectors
- mine: N/A

So each version (yours or mine) has some pros and cons. I think it might be good to combine them. If you want I can have a go at that.

Matlab accepts column or row vectors anywhere; AFAICS most of the functions in your repo only accept column vectors.
I'd like most of these functions to accept row and column vectors, or Nx2 (XY) or Nx3 (XYZ) arrays (and where applicable return row and column vectors or arrays depending on input).

Philip Nienhuis <philipnienhuis>
Project Member
Tue 21 Nov 2017 06:58:54 AM UTC, comment #4:

JuanPi: yep I'll check.

In the meantime, if you like you can check the mapping repo:
http://hg.code.sf.net/p/octave/mapping
for polybool, polyjoin, polycut and ispolycw.

I've tried at great lengths to accept row & column vectors and NxY matrices in most of these funcs where applicable. ML's mapping toolbox accepts row vectors and row cell vectors about everywhere it seems.

Another thing I'm thinking about is the poly2fv function written by Amir last year. It is meant to prepare for plotting polygons with holes and is probably much better (and faster) than polycut. It is a very useful function.
As it stands it's based on the polytri library and -unfortunately- Boost. But maybe there's a way to extract just the required Boost headers rather than the full 110 MB download.
In addition it has insufficient input error catching and internal error catching. Maybe around next Xmas I can try to polish my (somewhat rudimentary) C++ skills ...

Philip Nienhuis <philipnienhuis>
Project Member
Mon 20 Nov 2017 10:16:10 PM UTC, comment #3:

Hi Philip,

I think you need to check those other heads, there is already a folder with matlab compatible wrappers. Please check the repo in the link I sent you, in particular this folder.

https://sourceforge.net/p/octave/geometry/ci/2be7484d72c9cf0c021c16a5a9852e23678792dd/tree/inst/funcAliases/

Do my changes make sense? or shall I revert?

Cheers

Juan Pablo Carbajal <juanpi>
Project MemberIn charge of this item.
Mon 20 Nov 2017 04:33:11 PM UTC, comment #2:

Some answers:

- The whole file had CRLF end-of-lines that were changed into just LF by the cset.
I suppose they now still have CRLF? or was that a glitch in my local repo, could be.

- I did a pull ("hg -v pull" and "hg -v update -C") before creating the cset. Afterwards I was warned about multiple heads, one regarding a huge commit, the other regarding _dbl2int64_. I have no idea where those multiple heads come from.

- joinPolygons passed all 4 tests on my box. ???

- I think joinPolygons should either accept two column vectors (X and Y) OR just one Nx2 (XY) or Nx3 (XYZ) matrix. It shouldn't accept two matrices or a matrix and a vector.
Would you like me to add another input check for that case?

On a related note, I provisionally added polybool, polycut and polyjoin to mapping (all Matlab compatibility wrappers for similar functions in geometry) but I'd be equally happy (or even slightly happier) if they lived in the geometry package. I needed them on short notice so they're just in my local build.
A while ago you wrote that maybe a Matlab-comaptible subdir in geometry could host those function files. Still want that? if so I can prepare a patch for that as well as soon as I know a name for such subdir.

Either way, I'd like to make a mapping pkg release before the end of 2017 as I have fixed many bugs in the shapefile routines and several new functions were added. As mapping depends on geometry and some patches are required for geometry (polygon2patch & joinPolygons), maybe a maintenance release of geometry would be in order as well. What do you think of that?

Philip Nienhuis <philipnienhuis>
Project Member
Sun 19 Nov 2017 10:44:12 PM UTC, comment #1:

I started working with the cset for joinPolygons.

I found the some problems, so I did the following and pushed these changes (hopefully they implemente the desired functionality).

- The cset changes the whole file when actually only 2 lines changed

- I do not find the parent revision nor the node ID in the log of the repository

- With the new input checking a previous test fails (I fixed the input checking, please check):

- The function works with the following input, but I do nto what the output means- Maybe this input shouldn't be accepted

Maybe the input should be rejected on the basis that the cell array is not vertical. This would be in accordance with the help text.

The changes I made are in 2be7484d72c9
https://sourceforge.net/p/octave/geometry/ci/2be7484d72c9cf0c021c16a5a9852e23678792dd/tree/

Juan Pablo Carbajal <juanpi>
Project MemberIn charge of this item.
Sun 19 Nov 2017 07:32:45 PM UTC, original submission:

Hi JuanPi,

After a very long spell I finally got to updating the mapping package and connect the polygon operations (clipping etc) to the relevant functions we've put in the geometry package last Spring.

Along the way I hit two buglets in functions I supplied. Could you please review them and if accepted, apply them? or I can apply if you've reviewed them.

Thanks
Philip

Philip Nienhuis <philipnienhuis>
Project Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #42562:  polyjoin_dec3.cset added by philipnienhuis (7KiB - application/octet-stream)
file #42563:  polygon2patch_dec3.cset added by philipnienhuis (879B - application/octet-stream)
file #42439:  polygon2patch.cset added by philipnienhuis (879B - application/octet-stream)
file #42440:  joinPolygons.cset added by philipnienhuis (4KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by juanpi (Posted a comment)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by philipnienhuis (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-03 philipnienhuis Attached File- => Added polyjoin_dec3.cset, #42562
        Attached File- => Added polygon2patch_dec3.cset, #42563
    2017-11-19 philipnienhuis StatusNone => Ready For Test
        Carbon-Copy- => Added juanpi
    2017-11-19 philipnienhuis Attached File- => Added polygon2patch.cset, #42439
        Attached File- => Added joinPolygons.cset, #42440

    Back to the top


    Powered by Savane 3.3