bugGNU Octave - Bugs: bug #57500, [octave forge] (image) imrotate...

 
 

bug #57500: [octave forge] (image) imrotate fails in default branch

Submitter:  Avinoam Kalma <avinoam>
Submitted:  Sun 29 Dec 2019 09:58:29 PM UTC
   
 
Category:  Octave Package Severity:  4 - Important
Priority:  5 - Normal Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  carandraug
Originator Name:  Avinoam Open/Closed:  * Closed
Release:  * other Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 27 Jan 2020 02:08:14 AM UTC, comment #7: 

I have now removed the valid argout with http://hg.code.sf.net/p/octave/image/rev/81fba1b99154

Thank you

Carnë Draug <carandraug>
Group Member
Sun 26 Jan 2020 09:51:28 PM UTC, comment #6: 

I am also fine with removing this "valid output" feature.

Reasoning: There are in principle three options:

  • leave the code mostly as it is, just maybe document it better (my comment #4.) Carne thinks this is not a good option (his comment #5), I can even follow his argumentation. It is currently not a very useful (if at all) Octave-only feature.


  • improve this feature to make it work in meaningful use cases. So far I have not read that anyone values this feature enough to spend the effort to implement this.


  • remove this feature. I have read Avinoam as well as Carne prefering this option. And I'm fine with it as well.


So let's remove it. Have I correctly understood that you offered to implement this removal (comment #5), Carne? Or would you prefer that someone else proposed as patch for it?

Hartmut <hardy>
Sat 25 Jan 2020 12:30:58 PM UTC, comment #5: 

If we want to keep the "valid" feature, we should at least:

  • check if "valid" is argout and default extrapvalue and error if not appropriate for the image type
  • we could fix the isna for something appropriate to the user choice of extrap value.


However, this feature has been broken for a long time and no one reported it which leads me to believe it may not be used at all. If it's not used, that's a good argument to remove it and have less code to maintain, which means more maintenance time to improve other features that may be more useful.

Obviously I am biased for the functions of the package that I do use, so if you believe this is useful, go for it. If not, I would remove the extrapvalue arguemnts and valid  output arguments.

Carnë Draug <carandraug>
Group Member
Thu 23 Jan 2020 08:08:55 PM UTC, comment #4: 

Sorry, second thoughts. Here is my new suggestion:

  • Let's leave the code as it is right now. (Including Carne's patch that puts the "valid" output back in.)
  • Add a sentence to the doc string of imrotate (and probably imremap and imperspectivewarp) to explain that this valid output only works for floating point input images, and that you need to set the extrapval to NaN for it to work properly


This way

  • We stay fully backwards compatible.
  • We properly document what the image package can do (and what it can't).
  • We keep a (small) Octave-only feature.


How do you like this proposal? In case you do like it, I could prepare a patch for it (unless native english speakers prefer to do it themselfs.)

Hartmut <hardy>
Thu 23 Jan 2020 07:49:17 PM UTC, comment #3: 

Yes, I think this valid output of imrotate never ever worked as expected:

  • To find these in"valid" pixels, imremap searches for NaN values in its "warped" variable.
  • "warped" is generated by Octave's interp2 using the parameters "interp" and "extrapval". In order to ever produce any NaN values, interp2 needs to get extrapval=NaN as calling parameter.
  • The extrapval parameter in imremap has a default value of 0. So it would need to be explcitly set to NaN when imremap is called.
  • imremap is called by imperspectivewarp. The the parameter "extrapolation_value" in imperspectivewarp would need to be equal to NaN. (It's default value is also zero here.)
  • imrotate is calling imperspectivewarp. So it's parameter "extrapval" would need to be NaN. (It's default value is also zero.)


So there seems to be only one way to get useful output of "valid" from imrotate: You need to call imrotate with "extrapval=NaN".

The case Carne mentioned, where there is no easy workaround for a removed valid output would be integer images (that do not have a NaN value).

I was suspecting (as Carne did) that even in this case, the valid output would not work. Because the etrapvalue of NaN would be of different class than the uint class of the image, and somewhere this extrapvalue must be cast to the class of the image when both number should go into the same final output image.

That's why I tried the following (in image 2.10.0 without any of those changes):


>> I = ones(5);
>> I = uint8(I)
I =

  1  1  1  1  1
  1  1  1  1  1
  1  1  1  1  1
  1  1  1  1  1
  1  1  1  1  1

>> [I2, ~, valid] = imrotate (I, 45, "nearest", "loose", NaN)
I2 =

  0  0  0  0  0  0  0
  0  0  0  1  0  0  0
  0  0  1  1  1  0  0
  0  1  1  1  1  1  0
  0  0  1  1  1  0  0
  0  0  0  1  0  0  0
  0  0  0  0  0  0  0

valid =

  1  1  1  1  1  1  1
  1  1  1  1  1  1  1
  1  1  1  1  1  1  1
  1  1  1  1  1  1  1
  1  1  1  1  1  1  1
  1  1  1  1  1  1  1
  1  1  1  1  1  1  1

>>


My conclusion is, that even in this only possible case, where the valid output might have been useful, it obviously doesn't do anything. So there is and was never ever was any useful output of this "valid" output variable in imrotate.

My suggestion: Let's right now REMOVE this valid output variable (as discussed earlier in this bug report.) We could make a note in the NEWS file that we removed a parameter that never worked before (as suggested by Carne in comment #2).

What do you think?


Hartmut <hardy>
Thu 23 Jan 2020 02:00:13 PM UTC, comment #2: 

Sorry, I missed this discussion. I only noticed the issue now and put the valid argument back with http://hg.code.sf.net/p/octave/image/rev/04ab95ae4750 and linked to bug #55780 because that's where the patch that introduced the bug came from.

I don't oppose the removal of the valid argument but we should go through a phase of deprecation. We could see if at least anyone complains about it.

The easy workaround of specifying a extrapolation value is not that easy.  If the image is of an integer type, then there's no NA or NaN that could be used to distinguish valid elements.  This works inside imremap because it converts the image to double before interpolation, but then it casts it back to integer before returning to the user which will cause NA to be cast to zero.

Despite all this, looking at this again, this time with the extrapolation value argument in mind, I see that this probably never worked properly. The default extrapolation value is zero all over the place.  However, imremap uses `isnan` independent of whatever extrapolation value. If this is correct, we guess we can just remove all this and leave a comment on the NEWS that the options were removed and probably never worked?

Carnë Draug <carandraug>
Group Member
Thu 02 Jan 2020 04:23:04 PM UTC, comment #1: 

I have looked into this a bit:

At first I did not like to abandon a nice Octave-only feature, as is this "valid" output matrix of the imrotate function. But now I think this is the price we need to pay to fix bug #55780 effectivly.

On the positive side of removing this feature I see:

  • We get rid of some code duplication in the code, by now using the interpolation method from core Octave in interp2 directly.
  • There is an easy workaround for people who actually used this Octave-only feature of the "valid" output in imrotate: Just use an extrapolation value of NaN, and afterwards do something like valid = !isnan(im).


So as a conclusion, I would support Avinoam's proposal from comment #0. I.e. remove this "valid" output feature.

Once we remove this feature we should definitly make a note in the NEWS file, and maybe also mention the easy workaround sketched above.

My open question:

  • Can we do this (removal of an Octave-only feature) right away, or should we first deprecate it and warn users about this? (And remove this feature only in the second next release? And marking the failing BIST in imrotate as known failure for the meantime.) I personally tend to remove this feature straight away, without any prior deprecation warning. I expect that not many people actually use this feature. What do other people think of this? Carne?
Hartmut <hardy>
Sun 29 Dec 2019 09:58:29 PM UTC, original submission:  


While fixing bug #55780, with http://hg.code.sf.net/p/octave/image/rev/226a0e30ebc4,
the definition of second variabe of imremap (valid) was removed.
This cause failure in imrotate BIST.

I suggest to remove "valid" from imremap.m, imperseptivewarp.m and imrotate.m (Note that valid parameter is not matlab compatible).

Avinoam Kalma <avinoam>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

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
  • -email is unavailable- added by avinoam
  • -email is unavailable- added by avinoam (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 group members can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-01-27 carandraug StatusNone Fixed
        Open/ClosedOpen Closed
    2019-12-29 avinoam Carbon-Copy- Added carandraug
        Carbon-Copy- Added hardy

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code