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

 
 

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

Submitted by:  Martin Janda <tojtoj>
Submitted on:  Mon 03 Dec 2018 12:41:27 AM UTC  
 
Category:  Forge : new function Priority:  5 - Normal
Status:  In Progress Privacy:  Public
Assigned to:  None 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)

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>
Project 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>
Project 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>
Project 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 #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
  •  

    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 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    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 carandraug@octave.org

    Back to the top


    Powered by Savane 3.5