[Insight-developers] [Insight-users] Image offset is giving bad pointer for large datasets (7Gb)
Luis Ibanez
luis.ibanez at kitware.com
Mon Jul 13 17:45:07 EDT 2009
Hi Brad,
I agree, if we are going to make this pass, it is worth to take
advantage of it for fixing other related issues.
With the current changes, we will be using size_t for the number
of pixels in an image makes sense. Which as you pointed out, in
a 32-bits platform may not be good enough if we distribute the
image across multiple machines.
The next option is to use "long long", but my recollection is that
"long long" is even less portable than "long".
Any C++ type multi-platform Gurus could help here ?
Also, it would seem that whatever type we use for the number of
pixels, should be the same one used for the components of the
index: because it is valid to create a 1D image of a large
number of pixels, and therefore, each component should be able
to count up to the total number of pixels in the image.
Other types to look for during this review will be the Spacing and
Origin. In the fixes committed this weekend I found *many* examples
using floats for spacing and origin, which is misleading for users.
Immediate concerns are:
1) Cumulation types: E.g. sums of number of pixels
Histograms ?
2) NumericTraits for the new types
3) vnl math functions are not defined for __int64
(which is size_t and ptrdiff_t in Windows 64)
I'm sure that I'm missing many other implications.
You raise a good question:
Whether this should be done as part of a
major release instead of a minor release.
If we want to push this for 3.16 it will have to be done
in the next two or three weeks... and we probably won't
be able to do much of anything else.
So it is a matter of resources...
We could put this on hold,
recommend to users who need images larger than 2Gb that
they should use Linux 64 bits instead of Windows 64 bits,
and postpone this refactoring until ITK 4.0.
What other developers think ?
Luis
--------------------------
Bradley Lowekamp wrote:
> Luis,
>
> If we are questioning what types things should be, I'd like to add one
> more to consider during this pass. How many pixel are in a image? And
> what type should that be? The first thought would be size_t. This will
> work for many cases, but it does limit us. If we wanted to do
> out-of-core processing, this really should be a 64-bit value. A similar
> argument could be made for the IndexValueType, but that doesn't seem as
> useful.
>
> Are there other types we don't have right? Or are there other types that
> limit us, but we are ok with that limitation?
>
> Also is this the type of change that should be done on a minor revision
> or a major?
>
> Brad
>
> On Jul 13, 2009, at 2:51 PM, Luis Ibanez wrote:
>
>> 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 <mailto: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 <mailto: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 <mailto:blowekamp at mail.nih.gov>
>>
>>
>>
>
> ========================================================
>
> Bradley Lowekamp
>
> Lockheed Martin Contractor for
>
> Office of High Performance Computing and Communications
>
> National Library of Medicine
>
> blowekamp at mail.nih.gov <mailto:blowekamp at mail.nih.gov>
>
>
>
More information about the Insight-developers
mailing list