patchGNU Octave - Patches: patch #9712, [octave forge] (image) new...

 
 

patch #9712: [octave forge] (image) new function bwpack

Submitter:  Martin Janda <tojtoj>
Submitted:  Wed 31 Oct 2018 12:01:29 AM UTC
   
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  None Open/Closed:  Closed
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Wed 07 Nov 2018 01:04:14 PM UTC, comment #12: 

Functions should all work for N dimensions, this should not need to be mentioned, it's lack of support for N dimensions that should be mentioned.  And this difference does not cause compatibility issues with Matlab so there's no reason to mention it in relation to Matlab.

Carnë Draug <carandraug>
Group Member
Wed 07 Nov 2018 12:27:14 PM UTC, comment #11: 

Thanks for improving the code, it's much cleaner (I think you should be listed as author, if not the only one). I still suggest to mention multidimensional support in the docs, since this is where it differs from MATLAB.

Martin Janda <tojtoj>
Wed 07 Nov 2018 11:08:34 AM UTC, comment #10: 

Thank you for the patch. I have pushed it and it will be part of the next minor release (2.10.0). I made some changes to it, namely

1) instead of checking for the type, I simply cast to logical and catch any error. The thing is that bw functions typically just convert input to logical. In Matlab, logical() will error when converting complex values. However that works in Octave so it makes sense that bwpack also supports it.

2) I removed the note about bitpack. The purpose of that implementation note is not to explain variability or performance of the function, is simply to shift the blame for issues. We don't do it for all the other functions being used such as padarray and I don't think it makes sense.  Instead, can you fix bitpack?

3) I added more tests for N dimensional with different dimensions of zero length and a test for when the output will have more than 1 row.

Carnë Draug <carandraug>
Group Member
Thu 01 Nov 2018 08:44:18 PM UTC, comment #9: 

Thanks for help! I'm attaching an updated version bwpack_v2.patch that supports n-dimensional images as well. I put a note in the docs that it uses bitpack internally, hopefully offloading the responsibility of taking care of endianness to that function.

(file #45347)

Martin Janda <tojtoj>
Thu 01 Nov 2018 04:23:13 PM UTC, comment #8: 

Seems that in big-endian systems bitpack does not do what we need for bwpack.  See bug #54931

Still, that may be a bitpack bug so it should be fixed rather than worked around.

Carnë Draug <carandraug>
Group Member
Thu 01 Nov 2018 02:04:52 PM UTC, comment #7: 

I don't know of any big-endian machine I could test it on at the moment and probably won't get my hands on anything except AMD64 / x86-64 anytime soon (maybe I'm just now aware of any).

Martin Janda <tojtoj>
Thu 01 Nov 2018 01:04:12 PM UTC, comment #6: 

I'm not sure bitpack does the right thing. Its documentation says that the bits come in increasing order of significance but then they also say that x(1) is bit 0 which will not be the least significant bit for uint32 on big-endian.  Also, looking at the code, seems that they pack the values one byte at a time so I don't think the endianess is used.  If you have a big-endian machine running Octave, can you test it?

Carnë Draug <carandraug>
Group Member
Wed 31 Oct 2018 09:01:12 PM UTC, comment #5: 

Does bitpack/bitunpack take care of endianness? The documentation says the bits come in increasing order of significance so I expect if x(1) is always the least significant bit, bitpack should place it at the correct position in the output. Am I right?

Martin Janda <tojtoj>
Wed 31 Oct 2018 07:20:10 PM UTC, comment #4: 

Also, although matlab only supports 2D input, I think it would be nice to add support for more dimensions.  No reason to not pack stuff just because it is 3 or 4d.

Carnë Draug <carandraug>
Group Member
Wed 31 Oct 2018 07:18:52 PM UTC, comment #3: 

yes, I think it can still go in the package even without the other functions yet implemented.  Note that there is also bitunpack if you plan on implement bwunpack.

Carnë Draug <carandraug>
Group Member
Wed 31 Oct 2018 01:07:46 PM UTC, comment #2: 

Hi, it doesn't accept images with more than two dimensions (it fails as well as MATLAB, there is a BIST test for that). Little/big endian - haven't thought of that, good point.

Should I use bitpack? Definitely! I didn't know the function existed.

I know the function is probably only useful with bwunpack, imdilate and imerode, nevertheless it can help someone (maybe even me) to add/extend these functions and it is on the list of missing functions. Can it make it into the package if it 1) works, 2) I modify it according to your suggestions (+improve coding style)?

Martin Janda <tojtoj>
Wed 31 Oct 2018 11:13:47 AM UTC, comment #1: 

How does bwpack handle images that are more than 2 dimensions? Does the result differ in big-endian and little-endian machines differ?

I think the code be made simpler by using bitpack. Something like this:


bitpack (BWP(:), 'uint32')


which may also make it faster since it removes the for loop.

You don't need to check if image islogical. Just cast it as logical and be done.

I remember once writing this function but never finished because I found some  with bitpack. I don't remember what but I still have a unfinished patch where I'm rewriting bitpack in core :/

I thought this function was only really useful with imerode and imdilate have support for packed images which they don't.

Carnë Draug <carandraug>
Group Member
Wed 31 Oct 2018 12:01:29 AM UTC, original submission:  

Hi, I implemented an Octave version of MATLAB bwpack function ( https://www.mathworks.com/help/images/ref/bwpack.html).

Martin Janda <tojtoj>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #45347:  bwpack_v2.patch added by tojtoj (3KiB - application/octet-stream)
file #45332:  bwpack.patch added by tojtoj (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 tojtoj (Submitted the item)
  • -email is unavailable- added by tojtoj
  •  

    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 logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-11-07 carandraug StatusNone Done
        Open/ClosedOpen Closed
    2018-11-01 tojtoj Attached File- Added bwpack_v2.patch, #45347
    2018-10-31 tojtoj Attached File- Added bwpack.patch, #45332
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code