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

 
 

patch #10095: [octave forge] (image) new function imshowpair

Submitter:  Martin Janda <tojtoj>
Submitted:  Sun 15 Aug 2021 10:59:51 PM 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 20 Dec 2021 08:59:40 PM UTC, comment #6: 
Avinoam Kalma <avinoam>
Group Member
Fri 17 Dec 2021 11:14:40 AM UTC, comment #5: 

Thanks, @Hartmut for the detailed review.

I will make the changes that you have suggested and push this function.

Avinoam Kalma <avinoam>
Group Member
Thu 16 Dec 2021 10:04:20 AM UTC, comment #4: 

I have eventually found the time to have a closer took into this new function contribution imshowpair.m. The following points are already very good:

  • I have tried all the code examples on the Mathworks help page to this function, and the results looked visually the same to me.
  • I have checked all allowed image input types, and they all worked properly.
  • I have checked all allowed input parameters, and they all work fine. Only the parameter "Interpolation" is not implemented yet, but this new functions works nevertheless and just emits a useful warning message.


In general I would be happy to see this new function pushed to the image repository. There are just a few things that should  probably be improved before:

  • There are two code lines with an akward line break (currently line 85 and 108), those should probably be fixed.
  • The help string for this new function is a bit too short. This probably be extended to a hand full of sentences to desribe what the function does (and not only refer to the help text of imfuse).
  • There are currently no tests at all included. Since this functions mostly only processes the input parameters and then calls imfuse.m, the tests could well be very few in this case, but there should be at least some.


Here is my suggestion for a simple test that only checks once for each type of input argument. It could be added at the bottom to the code of imshowpair.m:


%!test
%! A = uint8 (200.*rand (100));
%! B = uint8 (150.*rand (100));
%! RA = imref2d (size (A), 0.5, 0.5);
%! RB = imref2d (size (B), 0.5, 0.5);
%! figure;
%! Ax=axes;
%! assert (imshowpair (A, B));
%! assert (imshowpair (A, RA, B, RB));
%! assert (imshowpair (A, B, "blend"));
%! assert (imshowpair (A, B, "falsecolor", "ColorChannels", "red-cyan"));
%! assert (imshowpair (A, B, "Parent", Ax));
%! assert (imshowpair (A, B, "montage", "Scaling", "joint"));
%! close;


@Avinoam: Could you change the above small things, add my (or more if you like) small BIST test to the code, and then push it to the image repo?

@Martin Janda: Thanks for this nice peace of code for the image package.

Hartmut <hardy>
Sat 27 Nov 2021 06:03:56 PM UTC, comment #3: 

Ping:

@Martin: any news relating comment #2?
@Hartmut: could please review this patch?

Avinoam Kalma <avinoam>
Group Member
Fri 03 Sep 2021 11:50:38 AM UTC, comment #2: 

Looks good. Minor comments:

1. There is an identation problem in two lines
2. Could you please add some tests? (mainly to check input validity)?

Avinoam Kalma <avinoam>
Group Member
Mon 16 Aug 2021 04:32:38 AM UTC, comment #1: 

Looks good, Thanks!

Avinoam Kalma <avinoam>
Group Member
Sun 15 Aug 2021 10:59:51 PM UTC, original submission:  

Here is a quick'n'dirty incomplete version of the imshowpair function from the time when I started playing with it. If I remember correctly, it will require refactoring code used by imfuse. This version gives visually same results as the MATLAB online version. If you think I'm on the right track, I can invest more time in it.

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 #51786:  imshowpair.patch added by tojtoj (6KiB - application/octet-stream - imshowpair: first draft)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by hardy (Posted a comment)
  • -email is unavailable- added by avinoam (Posted a comment)
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by tojtoj (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 logged-in users can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-12-20 avinoam StatusIn Progress Done
        Open/ClosedOpen Closed
    2021-08-16 avinoam StatusNone In Progress
        Carbon-Copy- Added hardy
    2021-08-15 tojtoj Attached File- Added imshowpair.patch, #51786

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code