bugGNU Octave - Bugs: bug #38085, [octave forge] (image) fails to...

 
 

bug #38085: [octave forge] (image) fails to build with --enable-64 (entropyfilt)

Submitter:  Carnë Draug <carandraug>
Submitted:  Mon 14 Jan 2013 06:31:05 PM UTC
   
 
Category:  Octave Package Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Build Failure
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 03 Sep 2018 05:12:22 PM UTC, comment #17: 

Yes, you are right, the results of entropyfilt are wrong.
I will open a new bug report.

Avinoam Kalma <avinoam>
Group Member
Fri 31 Aug 2018 11:43:26 PM UTC, comment #16: 

The original, failing to build with 64 bit enabled has been fixed. So yeah, this can be closed.

However, when I looked at it, I found weird results with entropyfilt which have not been addressed. I think it still computes incorrectly. But no one has reported that so meh. Also, there's no tests to check if it's working properly.

Carnë Draug <carandraug>
Group Member
Sun 26 Aug 2018 04:41:20 PM UTC, comment #15: 

Does this bug still exist?

Avinoam Kalma <avinoam>
Group Member
Fri 22 Sep 2017 01:22:20 PM UTC, comment #14: 

Do I understand it right, that the main issue of this bug report (image package not compiling under 64bit Octave) is now FIXED? Can we close this bug report? Or what does still need to be done?

Hartmut <hardy>
Thu 02 Jun 2016 07:36:47 AM UTC, comment #13: 

Good morning to everyone.

It's been a time since last additions to this bug.
We have been using image package for 3D images several months and the previously proposed changes seem to give correct results; althought we have not developed specific tests. (maybe it would be possible to launch commands with a common .mat file and compare it with the results given by image 2.5.1 and Matlab ones).
What do you think about it?
In addition, for future releases, it compiles without issue with gcc 5.3.1 and -std=gnu++11.

Cheers, chechugarriga.

José García <chechugarriga>
Tue 07 Jul 2015 12:43:42 PM UTC, comment #12: 


> After some trials, it was possible to get the code to compile on Linux amd64 + gcc-4.9.2 changing those two lines to:
>
> hist (((int)((int)(vals (i)))) + ((int)add)) ++;
>
> hist ((((int)(int)(vals (i)))) + ((int)add)) /= (double)len;


More important than being able to compile the code, does it still return correct results? That function doesn't have any tests so it's hard for me to test it. I have confirmed that it is not Matlab compatible but that is by design (and will have to be fixed later).

You seem to care about this function so I can assume that you have test cases for this function. Can you please confirm that the function still behaves properly?

> As additional info, using bwlabeln needs 19 Gbytes of working mem RAM to be capable of processing 1,3 GigaVoxel grayscale (uint8) data. bwconncomp ++ labelmatrix needs "only" 5 Gbytes. Does it seem reasonable?


bwlabeln is known to use more memory as it uses union-find (it is meant to be faster than bwconncomp on specific types of images but I haven't come across of such type yet).  It also returns a matrix of class double (required for Matlab compatibility) so the return value of such image will be 10.4Gbytes. The way we work around border elements required a duplicate of the image.  There are other approaches but they'll be slower. I would appreciate if you could implement alternatives.

If bwconncomp is faster and more memory efficient for you, use it.

Carnë Draug <carandraug>
Group Member
Mon 06 Jul 2015 07:42:21 PM UTC, comment #11: 

Good evening to everyone,

On May we were trying to get a "linux 64bit integer build" to work with image package.

We were facing problems during the last 2 years with those kind of octave builds. It was pointed out by Jordi G.H. to make some changes in the  spatial.cc code.

My mistake;  at the moment of calling "make dist" to create the image.*.tar.gz it was not picking the spatial.cc that was modified. It is the first octave-forge package that we have edited and I was not capable of seeing that behaviour.

Anyway, I retried as Jordi explained, but it was also failing
 the same way, saying again about "ambiguous conversion". I'm not too much into programming so I tried to force to convert those entities into standard ANSI C "int" variable.


After some trials, it was possible to get the code to compile on Linux amd64 + gcc-4.9.2 changing those two lines to:




hist (((int)((int)(vals (i)))) + ((int)add)) ++;


hist ((((int)(int)(vals (i)))) + ((int)add)) /= (double)len;


Those changes seem acceptable?
We have tried with bwlabeln, bwconncomp, labelmatrix and others, and the system has not failed.
As additional info, using bwlabeln needs 19 Gbytes of working mem RAM to be capable of processing 1,3 GigaVoxel grayscale (uint8) data. bwconncomp ++ labelmatrix needs "only" 5 Gbytes. Does it seem reasonable?

If You feel like there are other tests or changes that should be done, please let me know.

Thanks, yours sincerely Jose

José García <chechugarriga>
Tue 12 May 2015 06:43:36 PM UTC, comment #10: 


>  By the way, if we comment two of the three alternatives on the include file .h from octave,would it be possible to test/ debug that entropy function is working as expected ?


I'm not even sure that entropyfilt works correctly on 32-bit machine. The code on 2.4.0, in a 32 bit build, sometimes segfaults when image is of signed integer (already fixed). I have also managed to get into an endless loop sometimes.

This function has no tests to begin with. A quick comparison against Matlab already shows me the results differ but I think a big part of this is how it does the padding. There may be other problems with the function but before this bug report, I have never looked at the function.

If you are only interested on getting the same results as in a 32 bit build, just try it with a test image yourself and check if the results are similar.

Carnë Draug <carandraug>
Group Member
Tue 12 May 2015 05:52:32 PM UTC, comment #9: 

Good evening everyone.
I tried with the change proposed by Jordi but the problem persists.
Tried on a fresh build of octave-4.0.0rc4.
It was said to change both lines to:

hist ((vals (i) + add).value ()) ++;

The problem is still present, gcc keeps informing of ambiguous alternatives.
By the way, if we comment two of the three alternatives on the include file .h from octave,would it be possible to test/ debug that entropy function is working as expected ?

Yours sincerely Chechu


José García <chechugarriga>
Tue 12 May 2015 05:44:30 PM UTC, comment #8: 

According to a user on the mailing list, Jordi's proposed fix does not fix it:

http://octave.1599824.n4.nabble.com/Is-enable-64-still-experimental-tp4669274p4670354.html

Carnë Draug <carandraug>
Group Member
Mon 16 Mar 2015 03:41:36 PM UTC, comment #7: 

The line numbers don't seem to match what I see in _spatial_.cc at revision faa0a913015f. Regardless, the problem seems to be with the following function:


template <class ET, class MT, class ET_OUT>
ET_OUT entropy_filt (MT &vals, octave_idx_type len, int nbins)
{
  ET add;
  get_entropy_info<ET> (add, nbins);

  // Compute histogram from values
  ColumnVector hist (nbins, 0);
  for (octave_idx_type i = 0; i < len; i++)
    hist (vals (i) + add) ++;
  for (octave_idx_type i = 0; i < len; i++)
    hist (vals (i) + add) /= (double)len;

  // Compute entropy
  double entropy = 0;
  for (octave_idx_type i = 0; i < nbins; i++)
    {
      const double p = hist (i);
      if (p > 0)
        entropy -= p * xlog2 (p);
    }

  return entropy;
}


A first weirdness is that it's templating the return value ET_OUT, yet it's always returning a double. I think the template argument ET_OUT is probably unnecessary, or else entropy should be changed to type ET_OUT so that it can use both float and double types.

As to the actual bug, I assume the problem is that `vals (i) + add` is returning octave_int8 values, but those can't be used for indexing. I don't even know why this used to work. I can't see what conversion path was used when octave_idx_type was int.

I think a simple fix might be to change those lines to


hist ((vals (i) + add).value ()) ++;


Jordi Gutiérrez Hermoso <jordigh>
Group Member
Sun 15 Mar 2015 04:15:51 AM UTC, comment #6: 

Mike: That is what I thought. Just checking for the record.

Michael Godfrey <godfrey>
Group Member
Sun 15 Mar 2015 03:10:44 AM UTC, comment #5: 

Packages can be built and installed for both, but no a package built for 32-bit-indexed Octave won't work with 64-bit-indexed Octave, and vice versa.

Mike Miller <mtmiller>
Group Member
Sun 15 Mar 2015 02:36:30 AM UTC, comment #4: 

Right, but can a package built with 64-bit indexing run
with 32-bit indexing Octave in any case?

Michael Godfrey <godfrey>
Group Member
Sun 15 Mar 2015 02:28:31 AM UTC, comment #3: 

No, this is not a release blocker. This relates to the image package anyway, not to any failure within Octave.

Mike Miller <mtmiller>
Group Member
Sun 15 Mar 2015 02:20:06 AM UTC, comment #2: 

Mike: Just a clarification: is 64-bit integer build
supported for the 4.0 release? I thought that it is
not.

So if 64-bit is not not supported, this needs fixing,
but not before 4.0 release.

Michael Godfrey <godfrey>
Group Member
Sun 15 Mar 2015 02:00:44 AM UTC, comment #1: 

Confirmed, still fails to install using the current development branches of Octave (built with 64-bit index type) and the image package. At a glance the error messages look the same.

Mike Miller <mtmiller>
Group Member
Mon 14 Jan 2013 06:31:05 PM UTC, original submission:  

The following bug was reported on the mailing list by Fredrik Lingvall <fredrik.lingvall@gmail.com>. Original can be seen on nabble.




I have Octave configured with --enable-64 and I can't get the image package to build with this setup:


octave:6> pkg install -forge image
__spatial_filtering__.cc: In function 'ET_OUT entropy_filt(MT&, octave_idx_type, int) [with ET = octave_int<signed char>, MT = intNDArray<octave_int<signed char> >, ET_OUT = double, octave_idx_type = long int]':
__spatial_filtering__.cc:558:9:   instantiated from here
__spatial_filtering__.cc:190:5: error: conversion from 'octave_int<signed char>' to 'long int' is ambiguous /usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:783:3: note: candidates are: octave_int<T>::operator float() const [with T = signed char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:781:3: note:                 octave_int<T>::operator double() const [with T = signed char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:777:3: note:                 octave_int<T>::operator T() const [with T = signed char]
__spatial_filtering__.cc:192:5: error: conversion from 'octave_int<signed char>' to 'long int' is ambiguous
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:783:3: note: candidates are: octave_int<T>::operator float() const [with T = signed char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:781:3: note:                 octave_int<T>::operator double() const [with T = signed char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:777:3: note:                 octave_int<T>::operator T() const [with T = signed char]
__spatial_filtering__.cc: In function 'ET_OUT entropy_filt(MT&, octave_idx_type, int) [with ET = octave_int<unsigned char>, MT = intNDArray<octave_int<unsigned char> >, ET_OUT = double, octave_idx_type = long int]':
__spatial_filtering__.cc:560:9:   instantiated from here
__spatial_filtering__.cc:190:5: error: conversion from 'octave_int<unsigned char>' to 'long int' is ambiguous
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:783:3: note: candidates are: octave_int<T>::operator float() const [with T = unsigned char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:781:3: note:                 octave_int<T>::operator double() const [with T = unsigned char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:777:3: note:                 octave_int<T>::operator T() const [with T = unsigned char]
__spatial_filtering__.cc:192:5: error: conversion from 'octave_int<unsigned char>' to 'long int' is ambiguous
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:783:3: note: candidates are: octave_int<T>::operator float() const [with T = unsigned char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:781:3: note:                 octave_int<T>::operator double() const [with T = unsigned char]
/usr/include/octave-3.6.2/octave/../octave/oct-inttypes.h:777:3: note:                 octave_int<T>::operator T() const [with T = unsigned char]
make: *** [__spatial_filtering__.oct] Error 1
'make' returned the following error: make: Entering directory
`/tmp/oct-y3b3iW/image-1.0.15/src'
mkoctfile -Wall __spatial_filtering__.cc
make: Leaving directory `/tmp/oct-y3b3iW/image-1.0.15/src'
error: called from `pkg>configure_make' in file
/usr/share/octave/3.6.2/m/pkg/pkg.m near line 1384, column 9
error: called from:
error:   /usr/share/octave/3.6.2/m/pkg/pkg.m at line 826, column 5
error:   /usr/share/octave/3.6.2/m/pkg/pkg.m at line 383, column 9


Carnë Draug <carandraug>
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

Digest:
   bug dependencies.

 

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 jwe (Updated the item)
  • -email is unavailable- added by chechugarriga (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by carandraug (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-31 carandraug StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2017-08-13 jwe Summaryimage package: fails to build with --enable-64 (entropyfilt) [octave forge] (image) fails to build with --enable-64 (entropyfilt)
    2015-05-12 carandraug Summaryimage package: fails to build with --enable-64 image package: fails to build with --enable-64 (entropyfilt)
    2015-03-15 mtmiller Dependencies- bugs #43908 is dependent
    2015-03-15 mtmiller StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code