[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Octave-patch-tracker] [patch #9730] [octave forge] (image) new function
[Octave-patch-tracker] [patch #9730] [octave forge] (image) new function imfuse
Wed, 17 Jun 2020 08:33:35 -0400 (EDT)
Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:77.0) Gecko/20100101 Firefox/77.0
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
* 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
* 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:
Message sent via Savannah
- [Octave-patch-tracker] [patch #9730] [octave forge] (image) new function imfuse,