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

 
 

patch #9722: [octave forge] (image) new objects imref2d and imref3d

Submitter:  Martin Janda <tojtoj>
Submitted:  Thu 15 Nov 2018 12:26:08 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

Mon 04 Feb 2019 09:32:22 PM UTC, comment #14: 

All 4 patches where pushed, started in
http://hg.code.sf.net/p/octave/image/rev/0fd0783b2a43
If there are still problems/bug, please use the bug tracker.

Closing

Avinoam Kalma <avinoam>
Group Member
Fri 01 Feb 2019 07:12:09 PM UTC, comment #13: 

Thanks for the information, that those tests should all be Matlab compatible.

I have now reviewed this code as much as I will be able to do (even in the next months). But since these are very many lines of code, this review has by no means been exhaustive.

And to my understanding Avinoam has also reviewed this code, already.

So my suggestion would be to now PUSH these new functions. Unless someone else (Carne?) has the time to do more reviewing.

Hartmut <hardy>
Fri 01 Feb 2019 05:11:11 PM UTC, comment #12: 

I hope they are :) I tested all of the examples online on their documentation sites (e.g. https://www.mathworks.com/help/images/ref/imref2d.html). You can run any code from the browser if you select Try This Example. I expect it should be Matlab R2018b compatible (according to the docs).

Martin Janda <tojtoj>
Fri 01 Feb 2019 04:40:50 PM UTC, comment #11: 

Sorry, I didn't know how to test those @xxx functions either. You seem to be right Martin, when I do it as explained in your comment #10, then all tests pass for me as well. Good.

There seem to be quite same tests in place, already. Very nice.

Are those tests all Matlab compatible?

Hartmut <hardy>
Sat 26 Jan 2019 09:59:23 PM UTC, comment #10: 

I have not enough experience with Octave so it might be a problem on my side. I'm testing with Octave 4.4.1 under Windows. The only way I was able to run the tests was from the inst directory specifying the containing folder like this

>> test @imref2d/imref2d
PASSES 32 out of 32 tests
>> test @imref2d/intrinsicToWorld
PASSES 9 out of 9 tests
>> test @imref2d/contains
PASSES 10 out of 10 tests
>> test @imref2d/sizesMatch
PASSES 6 out of 6 tests
>> test @imref2d/worldToSubscript
PASSES 7 out of 7 tests
>> test @imref3d/imref3d
PASSES 37 out of 37 tests
>> test @imref3d/contains
PASSES 11 out of 11 tests
>> test @imref3d/intrinsicToWorld
PASSES 13 out of 13 tests
>> test @imref3d/worldToIntrinsic
PASSES 10 out of 10 tests
>> test @imref3d/worldToSubscript
PASSES 11 out of 11 tests


Is this a good approach or should simple 'test imref2d' called from the inst directory work? I tried to call 'test strel' from inst/@strel directory and  I also got an error

>> test strel
error: matrix cannot be indexed with .
error: called from
    numel at line 23 column 3
    test at line 305 column 3


while calling it from the inst directory works

>> test @strel/strel
PASSES 28 out of 28 tests


Extending imshow is beyong the scope of this patch although it would be nice to do it for sure (I haven't tried it yet). There are more functions that should work with imref2d and imref3d  (I already use them in patch #9730).

Martin Janda <tojtoj>
Sat 26 Jan 2019 07:43:23 PM UTC, comment #9: 

Sorry, I've only managed to have a superficial look at this new code. But thanks already, it's quite some lines and adds a fully missing feature to Octave.

Here my comments:

  • I tried to do the first example from the matlab help page on imref2d. It works fine and gives the same results. But the final call to imshow with "imshow(im, RA)" then throughs an error. Would it be possible to also implement support for this think in imshow as well? (Maybe lateron?)
  • I have tried to run the unit tests. (It's very good that those exist.) Most tests in the 2d folder pass. Except the one for imref2d.m itself. There I see a failing test:


>> test imref2d
***** test
 r = imref2d;
 assert (r.XWorldLimits, [0.5, 2.5])
 assert (r.YWorldLimits, [0.5, 2.5])
 assert (r.ImageSize, [2, 2])
 assert (r.PixelExtentInWorldX, 1)
 assert (r.PixelExtentInWorldY, 1)
 assert (r.ImageExtentInWorldX, 2)
 assert (r.ImageExtentInWorldY, 2)
 assert (r.XIntrinsicLimits, [0.5, 2.5])
 assert (r.YIntrinsicLimits, [0.5, 2.5])
 r.ImageSize = [800, 600];
 assert (r.XWorldLimits, [0.5, 2.5])
 assert (r.YWorldLimits, [0.5, 2.5])
 assert (r.ImageSize, [800, 600])
 assert (r.PixelExtentInWorldX, 0.003333, 1e-5)
 assert (r.PixelExtentInWorldY, 0.0025)
 assert (r.ImageExtentInWorldX, 2)
 assert (r.ImageExtentInWorldY, 2)
 assert (r.XIntrinsicLimits, [0.5, 600.5])
 assert (r.YIntrinsicLimits, [0.5, 800.5])
!!!!! test failed
ASSERT errors for:  assert (r.PixelExtentInWorldX,0.003333,1e-5)

  Location  |  Observed  |  Expected  |  Reason
     ()           1         0.003333     Abs err 0.99667 exceeds tol 1e-05 by 1


  • In the 3d folder, I get only failing unit tests. Am I doing something wrong here?


>> test imref3d
***** error id=Octave:invalid-indexing imref3d().InvalidProperty
!!!!! error failed.
Expected id=Octave:invalid-indexing, but got <>


(My tests were done withh Octave 4.4.1 under Linux.)

Hartmut <hardy>
Wed 23 Jan 2019 07:19:09 PM UTC, comment #8: 

I would like to push  imref2d and imref3d.
Any comments?

Avinoam Kalma <avinoam>
Group Member
Thu 03 Jan 2019 11:01:31 PM UTC, comment #7: 

Well spotted, thanks. Attaching new patches with fixes.

(file #45857, file #45858)

Martin Janda <tojtoj>
Thu 03 Jan 2019 09:38:48 PM UTC, comment #6: 

Two comments:


Matlab:

>> R = imref2d([100 200], 0.1, 0.4)

R =

  imref2d with properties:

           XWorldLimits: [0.0500 20.0500]
           YWorldLimits: [0.2000 40.2000]
              ImageSize: [100 200]
    PixelExtentInWorldX: 0.1000
    PixelExtentInWorldY: 0.4000
    ImageExtentInWorldX: 20
    ImageExtentInWorldY: 40
       XIntrinsicLimits: [0.5000 200.5000]
       YIntrinsicLimits: [0.5000 100.5000]

Octave:

>> R = imref2d([100 200], 0.1, 0.4)
R =
imref2d with properties:

         XWorldLimits: [0.05 10.05]
         YWorldLimits: [0.2 80.2]
            ImageSize: [100 200]
  PixelExtentInWorldX: 0.1
  PixelExtentInWorldY: 0.4
  ImageExtentInWorldX: 10
  ImageExtentInWorldY: 80
     XIntrinsicLimits: [0.5 200.5]
     YIntrinsicLimits: [0.5 100.5]


it seems that m and n have swapped places in lines 111-112.


>> R = imref2d([100 200], [1.5 0.5], [2.5 0.5])


Matlab throws error because xWorldLimits(2) should be greater than xWorldLimits(1) (and of course, the same for yWorldLimits)

Avinoam Kalma <avinoam>
Group Member
Thu 03 Jan 2019 05:26:17 AM UTC, comment #5: 

I will review the new functions, do not rewrite them yet.

Avinoam Kalma <avinoam>
Group Member
Wed 02 Jan 2019 07:54:27 PM UTC, comment #4: 

I just didn't know how much reliable classdefs are. I remember being unable to call noparametric constructor with classdefs but that was with affine2d and affine3d if I remember correctly. I can rewrite it.

Martin Janda <tojtoj>
Wed 02 Jan 2019 07:35:27 PM UTC, comment #3: 


Why did you use old-style classes and not classdef?

Avinoam Kalma <avinoam>
Group Member
Mon 03 Dec 2018 12:53:51 AM UTC, comment #2: 

Attaching another changeset with a fix where I allow indexing vector properties of imref2d and imref3d.

(file #45568)

Martin Janda <tojtoj>
Thu 22 Nov 2018 09:06:22 PM UTC, comment #1: 

(adding Avinoam to cc)

Hartmut <hardy>
Thu 15 Nov 2018 12:26:08 AM UTC, original submission:  

Hi, I added new objects imref2d and imref3d implemented as old-style classes.

These classes exhibit i-sa relationship in MATLAB and I respected that. Some code could be inherited by imref3d and I didn't do it for the following reason: a call to 'class(r, "imref3d", imref2d_object)' creates an object with an imref2d field and then the imref3d would have only z-dimension fields. At first, I shared the code and delegated some calls to imref2d parent, but then I found out that 'filednames' function in MATLAB returns all fields for imref2d as well as imref3d objects. So I copied all x- and y- related fields from imref2d object to imref3d object in the constructor and duplicated all the functionality of imref2d in imref3d. imref2d is used only to preserve the is-a relashionship (the default imref2d instance sits in imref3d instance, unused).

Quite a long story but I wanted to describe the reasoning behind my decision. There might be a better solution, I'll be awaiting some feedback. So far as I know, the code is MATLAB-compatible.

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 #45857:  imref2d_imref3d_3.patch added by tojtoj (2KiB - application/octet-stream)
file #45858:  imref2d_imref3d_4.patch added by tojtoj (4KiB - application/octet-stream)
file #45568:  imref2d_imref3d_2.patch added by tojtoj (3KiB - application/octet-stream)
file #45427:  imref2d_imref3d.patch added by tojtoj (69KiB - 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 avinoam (Posted a comment)
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by hardy
  • -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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-02-04 avinoam Open/ClosedOpen Closed
    2019-02-04 avinoam StatusNone Done
    2019-01-03 tojtoj Attached File- Added imref2d_imref3d_3.patch, #45857
        Attached File- Added imref2d_imref3d_4.patch, #45858
    2018-12-03 tojtoj Attached File- Added imref2d_imref3d_2.patch, #45568
    2018-11-22 hardy Carbon-Copy- Added avinoam
    2018-11-15 tojtoj Attached File- Added imref2d_imref3d.patch, #45427
        Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code