[Octave-patch-tracker] [patch #9730] [octave forge] (image) new function imfuse

[Hartmut]

Follow-up Comment #4, patch #9730 (project octave):

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)?


    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/patch/?9730>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/