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

 
 

patch #9730: [octave forge] (image) new function imfuse

Submitter:  Martin Janda <tojtoj>
Submitted:  Mon 03 Dec 2018 12:41:27 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

Sun 15 Aug 2021 11:02:20 PM UTC, comment #24: 

@avinoam Hi, I submitted a new patch here: https://savannah.gnu.org/patch/index.php?10095

Martin Janda <tojtoj>
Sat 14 Aug 2021 08:44:21 PM UTC, comment #23: 

@martin: any news about imshowpair?
Thanks

Avinoam Kalma <avinoam>
Group Member
Mon 29 Mar 2021 11:35:16 AM UTC, comment #22: 

Pushed: http://hg.code.sf.net/p/octave/image/rev/9b909d9d49aa .
Thanks Martin, thanks Hartmut.

Avinoam Kalma <avinoam>
Group Member
Mon 15 Mar 2021 05:49:53 AM UTC, comment #21: 

Thanks @Hartmut for reviewing.
I will push it in the next few days.

Avinoam Kalma <avinoam>
Group Member
Sun 14 Mar 2021 05:16:24 PM UTC, comment #20: 

I have now had a closer look into the file imfuse.m that results when applying all patches from comment #17 (v4). As a conclusion I would be happy to see this file added to the image repo as it is now.

I have tested the following:

  • All examples from the Matlab support page produce (visually) the same results as Matlab does.
  • All current BIST tests pass. (I have not looked into the two failing xtests. Avinoam seems to have discussed this already.)
  • Most of my initial comments for improvement (comment #4) are now fine.
  • There are still no BIST tests for more exotic input types (int16, int32, ...) I have done a quick test for myself. Result: Using imfuse on an int16 image together with a int8 image gives an unintuitive result, but this result looks the same as in Matlab. Using two int16 input images give a visually useful result, this also looks like the one I get from Matlab. So (at least for int16) this code seems to work fine for other input types, anyways.


I have seem some minor style issues, that might be (only!) nice to change:

  • There a many line breaks in code lines. Some use "..." others use a backslash. Could be use the same kind of line continuation marks (e.g. always "...") on all of those?
  • I would like to have those line breaks at positions such that the code remains human readable. E.g. the current line 103 looks weired to me.
  • There are a few spaces missing before the opening brackets of a function. Some of them might well not be fixable. But the one on line 306 is.


@Avinoam. Yes, please go ahead and push this version of imfuse to the image repo, including the necessary bookkeeping changes.

@Martin Janda: Thanks a lot for implementing this nice and useful function for the Octave image package. (And if you should find the time for imshowpair, I would also be happy to see it!)

Hartmut <hardy>
Sun 28 Feb 2021 05:34:49 PM UTC, comment #19: 

Sorry for the delay. I thougt you guys were still trying to fix bug #51769 before returning to here.

I will try to review the new patch in comment #17 in two weeks time. (But I will most probably not find time to do this, during the next week.)

Hartmut <hardy>
Sun 28 Feb 2021 12:14:39 PM UTC, comment #18: 

I would like to push the last version of imfuse.
Any objection?

Avinoam Kalma <avinoam>
Group Member
Thu 11 Feb 2021 12:49:48 AM UTC, comment #17: 

Thanks, the d matrix was only a sanity check, not a serious assertion. I came up with a test that passes in MATLAB, see the attachment. The test passes in Octave too but the imfuse result is not numerically identical to the MATLAB one, probably due to the problems with imresize. Another failing test against a known constant matrix would be necessary as a reminder that the Octave version is incompatible. To look for such matrix in case of the checkerboard method is not worth my time (I tried to copy the output from the online version of MATLAB, unsuccessfully).

(file #50853)

Martin Janda <tojtoj>
Tue 09 Feb 2021 06:45:25 AM UTC, comment #16: 

I have checked the first failure.
There are two problems:

The first problem is in checkerboard_mask.
Actually, the problem is in imresize, and it seems to be bug #51769 & bug #57504.
A workaround, until we fix imresize, can be to change the line


c = logical (imresize (v, [m, n], "nearest"));


to:


  xi = (0.5:n-0.5)*16/n + 0.5;
  yi = (0.5:m-0.5)*16/m + 0.5;
  [XI, YI] = meshgrid (xi, yi);
  c = logical (imremap (v, XI, YI, "nearest"));


The 2nd problem is that


%! d = repmat ([black_tile, white_tile
%!              white_tile, black_tile], [16, 16]);
%! d = d (1:50, 1:50);


Is not the correct result.
 

Avinoam Kalma <avinoam>
Group Member
Sun 07 Feb 2021 10:25:54 PM UTC, comment #15: 

Thanks, I didn't notice. I've rewritten the (still failing) test.

Martin Janda <tojtoj>
Sun 07 Feb 2021 04:58:59 PM UTC, comment #14: 

Testing the new version, I got:


>> test imfuse
***** xtest
 a = 0.1 * ones (50, 50);
 b = 0.2 * ones (50, 50);
 c = imfuse (a, b, "checkerboard", "Scaling", "none");
 d = repmat ([26 * ones (3), 51 * ones (3)
              51 * ones (3), 26 * ones (3)], [16, 16])(1:50, 1:50);
 assert (sum (c(:) == d(:)), 1412);
!!!!! known failure
index (50,_): out of bound 32 (dimensions are 32x64)
....


because the result of

d = repmat ([26 * ones (3), 51 * ones (3)
              51 * ones (3), 26 * ones (3)], [16, 16]);


is a 32x64 matrix

Avinoam Kalma <avinoam>
Group Member
Sat 06 Feb 2021 12:41:12 AM UTC, comment #13: 

@Avinoam Thanks, looks good. I replaced the function, see patches-imfuse-2.zip

(file #50843)

Martin Janda <tojtoj>
Fri 05 Feb 2021 06:37:53 PM UTC, comment #12: 

@Martin: Not mandatory, just a recommendation. Thanks :-)

Avinoam Kalma <avinoam>
Group Member
Fri 05 Feb 2021 02:38:39 PM UTC, comment #11: 

@Avinoam Should I really replace is_image by the private function isimage? I didn't notice it exists before. They are slightly different though.

Martin Janda <tojtoj>
Fri 05 Feb 2021 01:29:51 AM UTC, comment #10: 

Here is a new version as a series of mq patches with most of Harmut's suggestions implemented. What has not been resolved yet:

  • more exotic input types (I might get to that in a forseeable future)
  • two of the new tests are failing and I have no idea why, MATLAT gives such output and a real world failing example might give me a better idea about what's going on (I doubt I'll do better without that example)


@Avinoam: Not to this patch, that might complicate and delay further the review but I'd still like to do so.

(file #50841)

Martin Janda <tojtoj>
Tue 02 Feb 2021 11:35:26 AM UTC, comment #9: 

@martin: Could you please add to the new patch an implementation of imshowpair?

Avinoam Kalma <avinoam>
Group Member
Tue 02 Feb 2021 01:11:32 AM UTC, comment #8: 

Hi there, yes, I couldn't force myself to find the time to recap up until today. I'm already working on it and will send a patch with some new tests and fixed code when I'm done.

Martin Janda <tojtoj>
Mon 01 Feb 2021 08:31:39 PM UTC, comment #7: 

@Martin Janda: Are you still planning to prepare a new patch for your imfuse function? It would be very nice if we could add this function to the image package as well.

Hartmut <hardy>
Thu 18 Jun 2020 07:16:22 PM UTC, comment #6: 

Thanks for your feedback, I'll prepare the patch.

Martin Janda <tojtoj>
Thu 18 Jun 2020 04:21:14 AM UTC, comment #5: 

@Hartmut

Thanks for your comprehensive review.

Two remarks:

  • The line a = [zeros (1, 100)] produces error, so we cannot reove all the spaces before the brackets.
  • I would like to replace is_image by the private function isimage.


If Martin Janda will not prepare a new patch, I will do it, but it will take some time to add new unit tests for those functions who don't have (including the one with the bug).

Avinoam Kalma <avinoam>
Group Member
Wed 17 Jun 2020 12:33:35 PM UTC, comment #4: 

I have eventually found some time to look at this new function "imfuse", sorry for the delay. First of all: Thank you, this looks like a nice and useful piece of code.

I have NOT looked at the logic of the code, I only checked the output results and formal things.

What is good:

  • All included test cases pass, except for the one xtest which is supoposed to fail.
  • I can run all examples from the Mathworks help page on their "imfuse" and get (visually) the same results. (I will undestand this as: "works in a Matlab compatible way".)
  • Octave coding style is (mostly) followed.



What is (mostly only minor) not so good:

  • The only major point first: There seems to be a wrong variable name (in a code line not touched by any test by the way) in line 329. Here the variable "imref.ImageSize" is used and does not exist. I think this should be changed to "rx.ImageSize"! (After changing this, also the very last Matlab example passed for me as well.)
    • idea: We might also add some simple test case, to make sure the code in this subfunction is also tested.
  • Some of the legal input syntaxes and data types are currently not checked by a unit test. It would be nice to add those as well:
    • The (default) method "falsecolor"
    • The colorchannel values "red-cyan" and "green-magenta"
    • The syntax with two output values: [C, RC] = imfuse(...)
    • A (3-channel) rgb image as input image (A or B)
    • Some more exotic input types for the images A and B, e.g. the signed integer types like "int16", "int32".
  • There is a wired line break (with "..." as line continuation) on line 100. Is this enforced by our coding style? Or could we make this more human readable by putting the whole code on one (slightly longer) line?
  • Coding style requires a space before the brackets when calling functions. This is mostly followed. But in some lines I have found missing spaces for the following function calls. It would be nice to search-replace those: uint8(), magic(), ones(), max(), isinteger(), double(), diff(), eye(), min(), zeros().


After (most of) the above changes, I would be very happy to see this code pushed to the image repo.

How do we proceed further? Could Martin Janda prepare a new patch? Or Avinoam? Shall I (in some time)?

Hartmut <hardy>
Fri 22 May 2020 05:56:20 AM UTC, comment #3: 

PING!
I would like to push imfuse.
Any objection?

Avinoam Kalma <avinoam>
Group Member
Wed 11 Mar 2020 04:48:28 AM UTC, comment #2: 

I have started checking this new function, and it looks very good.

Carne, Hartmut: Could you please also review imfuse. I would like to push it.

Avinoam Kalma <avinoam>
Group Member
Fri 07 Dec 2018 06:20:08 PM UTC, comment #1: 

(adding Avinoam to cc)

Hartmut <hardy>
Mon 03 Dec 2018 12:41:27 AM UTC, original submission:  

Here is a (usable) draft of the imfuse function, that should also serve as a good basis for imshowpair (I'm planning to add that too). Works with spatial referencing objects (not yet in Octave, see patch #9722). I've been looking at the code for way too long, I'd be grateful for another pair of eyes and maybe some additional failing tests.

Known issue: MATLAB produces a slightly different checkerboard mask, I haven't been able to come up with the same formula yet. It looks the same visually though. I doubt anyone will ever use this method anyway.

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 #50853:  patches-imfuse-4.zip added by tojtoj (15KiB - application/x-zip-compressed)
file #50845:  patches-imfuse-3.zip added by tojtoj (14KiB - application/x-zip-compressed)
file #50843:  patches-imfuse-2.zip added by tojtoj (13KiB - application/x-zip-compressed)
file #50841:  patches-imfuse.zip added by tojtoj (13KiB - application/x-zip-compressed)
file #45567:  imfuse.patch added by tojtoj (21KiB - 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 avinoam
  • -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 11 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-03-29 avinoam StatusIn Progress Done
        Open/ClosedOpen Closed
    2021-02-11 tojtoj Attached File- Added patches-imfuse-4.zip, #50853
    2021-02-07 tojtoj Attached File- Added patches-imfuse-3.zip, #50845
    2021-02-06 tojtoj Attached File- Added patches-imfuse-2.zip, #50843
    2021-02-05 tojtoj Attached File- Added patches-imfuse.zip, #50841
    2020-03-11 avinoam StatusNone In Progress
    2018-12-07 hardy Carbon-Copy- Added avinoam
    2018-12-03 avinoam Carbon-Copy- Added hardy
    2018-12-03 tojtoj Attached File- Added imfuse.patch, #45567
    2018-12-03 tojtoj Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code