bugGNU Octave - Bugs: bug #47502, [octave forge] (geometry or...

 
 

bug #47502: [octave forge] (geometry or mapping) polysplit and polyjoin functions

Submitter:  Alex Provan <provana>
Submitted:  Tue 22 Mar 2016 09:57:42 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Alex Provan Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 03 Sep 2017 01:00:19 PM UTC, comment #10: 

OK, closing report.
I think there are still some things to be discussed but that can be done on the maintainers ML.

Philip Nienhuis <philipnienhuis>
Group Member
Sun 13 Aug 2017 10:30:08 PM UTC, comment #9: 

These functions are in the GSOC 17 developer version. Will be in the next release.

Please close this bug

Juan Pablo Carbajal <juanpi>
Group Member
Thu 28 Apr 2016 10:16:28 PM UTC, comment #8: 

I am working on that as we speak. I will drop my local changes (mostly documentation and a few fixes) and keep close to upstream. I will slowly merge all my "code" and the upstream branch.

Juan Pablo Carbajal <juanpi>
Group Member
Thu 28 Apr 2016 09:58:31 PM UTC, comment #7: 

Yeah, ND may be too much.

MatGeom indeed seems to have a few more functions now than OF-geometry.

Philip Nienhuis <philipnienhuis>
Group Member
Thu 28 Apr 2016 08:59:38 PM UTC, comment #6: 

Yeap, that's true. Sorry I missed that.
What do oyu mean with ND joinPolygons? geometry (matGeom) is 2D,3D geometry package, so ND is out of the scope if you meant multiple dimensions.

Geometry is lagging behind matGeom and I am thinking to just submit all my upgrades upstream and start cloning matGeom. It will be good to have somebody checking the default branch to detect when I break backwards compatibility.

We can add the functions you deem necessary to geometry, no problem, as long as they stay within the scope of the package.

Juan Pablo Carbajal <juanpi>
Group Member
Thu 28 Apr 2016 06:58:33 PM UTC, comment #5: 

JuanPi,

I just found out that the OF geometry package already contains a function splitPolygons() that does exactly the same as polysplit.m.
polysplit() can process Lat,Lon as well as X,Y coordinates (but both are just numbers).

So, depending on speed (no benchmarks available yet) perhaps polysplit.m could even be a mere wrapper around splitPolygons() in geometry and be housed in the mapping package.


For polyjoin() there's no matching function in the geometry package. But it also works on general polygons.
So, maybe geometry could have an ND function joinPolygons() that would do the work for a 2D polyjoin.m in mapping? I already have a polyjoin.m that works on XYZ cell arrays (even XYZM).


In a more general sense, looking at the discussions and at the (limited) functionality (only 2D) of various polygon functions in the Matlab's mapping toolbox, I think the mapping package could well have a "hard" (instead of "suggested") dependency on geometry and contain Matlab-compatible functions that are mere wrappers for more capable functions living in the geometry package.

Maybe this could help the discussion about where to put what a bit forward. What do you think?

Philip Nienhuis <philipnienhuis>
Group Member
Thu 28 Apr 2016 05:08:42 PM UTC, comment #4: 

Hi,

Geometry and matGeom do pack several polygons in a cell array. Is this what you meant?

If the functions explicitly use mappings on the geoid, then I guess they should go to mapping.

If the functions are general for any kind of polygon, I guess then geomtry would be a good place. In any case we can always put the functions also in geometry, if they are worth the duplication.

Juan Pablo Carbajal <juanpi>
Group Member
Fri 15 Apr 2016 04:35:00 PM UTC, comment #3: 

Looking at the description of the files I am not very sure if the geometry package is the right place.
AFAICS these functions are mainly used for converting (X,Y) or (Lat, Lon) coordinates in mapstructs or geostructs to a form in which they can more easily be plotted, and back.  So they may be more useful in the mapping package after all.  Sorry for a bit of confusion.

JuanPi, what is your opinion? Does the geometry package use the cell array coordinate format?

@Alex:
=======
Thanks for your contribution.

Contributions like this should go on the patch tracker, not bug tracker.

Looking at polyjoin.m I see a for loop that had better be vectorized to avoid undue CPU time.
shapedraw.m in the mapping package invokes a better construct, I just translated it into 2D for you:

XY(1:2:2*numel (latcells), :) = [{latcells{:}}' , {loncells{:}}'];
XY(2:2:2*numel (latcells) - 1, :) = NaN;
XY = cell2mat (XY);
lat = XY(:, 1);
lon = XY(:, 2);


(BTW this trick is actually due to David Bateman)

The reverse operation in polysplit.m should also be amenable to vectorization. I'll have a look at it if no one beats me to it.

Furthermore, would you please follow Octave coding style?

- "A ~= B" should be "A != B"

- "~iscell" => "! iscell"

- two consecutive "|" rather than one

- indent only two spaces instead of four

... and please adapt the texinfo header to mention that X and Y coordinates can be treated as well. The Matlabs docs don't mention that but this is very useful.

Similar goes to polysplit.m.

Philip Nienhuis <philipnienhuis>
Group Member
Tue 29 Mar 2016 09:18:09 AM UTC, comment #2: 

As far as I am concerned, and given the maintainers ML discussion [1], they'd better go to the geometry package; that's where several other polygon functions already live.

The mapping package has a (suggested!) dependency on geometry (a.o., distancePoints, polygonArea, createLine, I probably overlook a few). It also depends on current oc_polybool from octclip, which I hope is to be replaced by a new function in geometry as well.

[1]
http://lists.gnu.org/archive/html/octave-maintainers/2016-03/msg00425.html
http://lists.gnu.org/archive/html/octave-maintainers/2016-03/msg00448.html

Philip Nienhuis <philipnienhuis>
Group Member
Sat 26 Mar 2016 07:37:51 PM UTC, comment #1: 

Have we decided already whether these functions go to geometry or mapping?

In the meanwhile:

1. I prefer (x ~= 2) and not  ~(x == 2)
2. Insert a blank space between "if" and conditions (Octave code style)
3. Introduce a blank space between any function and its arguments, e.g. "isempty (x)" (Octave code style)
4. Keep consistent indentation.
5. If you use octave terminators like "endfunction" please also use "endif", "endfor" etc...
6. Do not forget to always add your copyright notice to the header of your file.
7. Use texinfo for the docuemntation
8. Write tests
9. Write at least one demo.
10. Try to use Octave's error ids (check error_ids)

Check the new version of the function I attach. There is room for improvement in the documentation.






(file #36773)

Juan Pablo Carbajal <juanpi>
Group Member
Tue 22 Mar 2016 09:57:42 PM UTC, original submission:  

polysplit and polyjoin are two functions that do the reverse of each other. In MATLAB they are part of the vector data analysis section of the mapper toolbox package. I have written an implementation for each polysplit and polyjoin

polysplit:
converts line or polygon parts from vector form to cell array
form, essentially does the reverse of polysplit
returns the NaN ended segments of vectors latcells and loncells
one vector per cell of new cell arrays lat and lon

polyjoin:
converts line or polygon parts from cell array format to column vector format. in cell array format each cell defines a seperate polygon or line segment. returns two column vectors containing the polygons from the inputted cell arrays.

I have attached my implementations of these two functions for discussion, thanks

Alex Provan

Alex Provan <provana>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #36773:  polyjoin.m added by juanpi (2KiB - text/x-objcsrc)
file #36728:  polyjoin.m added by provana (1KiB - text/x-objcsrc)
file #36729:  polysplit.m added by provana (2KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by juanpi (Updated the item)
  • -email is unavailable- added by mtmiller (Updated the item)
  • -email is unavailable- added by provana (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-07-02 jwe Summary[octave-forge] (geometry or mapping) polysplit and polyjoin functions [octave forge] (geometry or mapping) polysplit and polyjoin functions
    2017-08-14 jwe StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2017-08-11 jwe Summarygeometry or mapping package: polysplit and polyjoin functions [octave-forge] (geometry or mapping) polysplit and polyjoin functions
    2016-04-16 philipnienhuis Carbon-Copy- Added philipnienhuis
    2016-03-26 juanpi Attached File- Added polyjoin.m, #36773
    2016-03-26 mtmiller CategoryOctave Function Octave Package
        StatusNone Patch Submitted
        Releasedev other
        SummaryFeature request: polysplit and polyjoin functions geometry or mapping package: polysplit and polyjoin functions
    2016-03-22 provana Attached File- Added polyjoin.m, #36728
        Attached File- Added polysplit.m, #36729

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code