[Insight-developers] [Insight-users] Image offset is giving bad pointer for large datasets (7Gb)

[Luis Ibanez]

Hi Brad,

                      You have said it all !  :-)


As usual... what it seemed to be a localized changed
turned out to be a massive refactoring.

We have promiscuously used "long", and "unsigned long"
all over the place, instead of having had the precaution of
declaring a Trait for the components of Size, Index and
Offset.  (now we have IndexValueType, SizeValueType,
and OffsetValueType).


The same goes for Spacing and Origin, but that's a bit
of a separate issue.


As you pointed out, we will have to search-and-destroy
all explicit uses of "long" and "unsigned long" that refer
to Index, Size and Offset components.


I believe that the changes applied during the weekend are
good for covering Linux in 32 and 64 bits, but unfortunately
they are still not enough for covering Windows 64 bits,
where


     size_t and ptrdiff_t have different size than "long".


Banning the use of "long" will come easy by simply keeping
a couple of Windows 64 bits Nightly builds, (once we
replace size_t and ptrdiff_t as the component types of
Size, Index and Offset.)

In the meantime, unfortunately we will have to go down
the painstaking process of cleaning class by class.


The process is not hard,
but it promises to be very long:

It comes down to getting a Visual Studio Windows 64 bits
build to run green, after we replace the new types.


It will be great to get this done by the Release of ITK 3.16,
but that may absorb too much of our resources.


I'll keep working on this as a background task...


      Luis


---------------------------
On Mon, Jul 13, 2009 at 11:30 AM, Bradley Lowekamp
<blowekamp at mail.nih.gov>wrote:

> Luis,
> This looks like an absolutely massive effort you are attempting here. I
> have grepped through the iterator classes are there alot of incorrect usages
> of long:
>
> itkImageRegionReverseConstIterator.h:    m_SpanEndOffset =
> this->m_BeginOffset - static_cast<long>(this->m_Region.GetSize()[0]);
>
> And then there are complex relations like this:
>
> itkSize.h:  typedef   unsigned long     SizeValueType;
> itkImageRegion.h:  typedef Size< itkGetStaticConstMacro( ImageDimension ) >
>  SizeType;
> itkImageRegion.h:  typedef typename SizeType::SizeValueType
>  SizeValueType;
> itkImageRegion.h:  SizeValueType GetNumberOfPixels() const;
>
>
> I am not really sure how all these issues can be tracked down. From the
> user list:
>
>
> On Jul 10, 2009, at 12:46 PM, Michael Jackson wrote:
>
>
> IMNSHO, ITK (and VTK by extension) should absolutely BAN the use of
> "long" in their projects. Period. Too many avoidable bugs come up from
> its use. If you want a 32 bit integer use int or the standard ansi int
> type, if you want a 64 bit integer then use "long long int" or the
> standard ansi 64 bit integer type.
>   "long int" is just a mess waiting to happen. Maybe a rule should be
> put into the KWStyle project to look for and flag the use of 'long'?
>
>
>
> Do we need to go as far as banning the use of long and unsigned long?
>
>
> Good Luck Luis,
> Brad
>
>
>
> On Jul 10, 2009, at 6:33 PM, Luis Ibanez wrote:
>
> On recent realization:
>
>              "size_t"    is unsigned       :-/
>
> and we need the OffsetValueType to be signed,
> since we use it to compute differences...
>
> It seems that what we need is the type
>
>                         "ptrdiff_t"
>
> which is supposed to represent the differences
> between two pointers, and therefore should be
> capable of measuring distances between any
> two locations in memory.
>
> I'm now rerunning the Experimental with
> "ptrdiff_t" instead of "size_t".
>
>
>        Luis
>
>
> ------------------------------------------------------------------------------------------------------
> On Fri, Jul 10, 2009 at 12:30 PM, Luis Ibanez <luis.ibanez at kitware.com>wrote:
>
>> Hi Kana,
>>
>> Thanks a lot for looking into this and sharing your findings.
>>
>> I just confirmed that in Windows 64bits, the "long" type is
>> only 4 bytes.
>>
>> Here is the program I used:
>>
>> #include <iostream>
>> #include "itkOffset.h"
>> #include "itkNumericTraits.h"
>>
>> int main()
>> {
>>   unsigned long tt;
>>   std::cout << "size = " << sizeof(tt) << std::endl;
>>   tt  =  -1;
>>   std::cout << "tt = " << tt << std::endl;
>>
>>   typedef itk::Offset<3>   OffsetType;
>>   typedef OffsetType::OffsetValueType   OffsetValueType;
>>
>>   OffsetValueType  offsetValue;
>>
>>   std::cout << "sizeof(offsetValue) = " << sizeof( offsetValue ) <<
>> std::endl;
>>
>>   offsetValue = itk::NumericTraits< OffsetValueType >::max();
>>
>>   std::cout << "OffsetValueType max() = " << offsetValue << std::endl;
>>
>>   return EXIT_SUCCESS;
>> }
>>
>>
>> with this CMakeLists.txt file
>>
>>
>> CMAKE_MINIMUM_REQUIRED(VERSION 2.4)
>> IF(COMMAND CMAKE_POLICY)
>>   CMAKE_POLICY(SET CMP0003 NEW)
>> ENDIF(COMMAND CMAKE_POLICY)
>>
>>
>> PROJECT(64bitsTest)
>>
>> FIND_PACKAGE(ITK REQUIRED)
>> INCLUDE(${ITK_USE_FILE})
>>
>> ADD_EXECUTABLE(typesTest typesTest.cxx )
>>
>> TARGET_LINK_LIBRARIES(typesTest ITKCommon)
>>
>>
> <ATT00001.txt>
>
>
> ========================================================
>
> Bradley Lowekamp
>
> Lockheed Martin Contractor for
>
> Office of High Performance Computing and Communications
>
> National Library of Medicine
>
> blowekamp at mail.nih.gov
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.itk.org/mailman/private/insight-developers/attachments/20090713/1da2a532/attachment.htm>