[Insight-developers] [Insight-users] Image offset is giving bad pointer for large datasets (7Gb)
Bradley Lowekamp
blowekamp at mail.nih.gov
Mon Jul 13 15:18:36 EDT 2009
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
> > 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
>
>
>
========================================================
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/cc194653/attachment.htm>
More information about the Insight-developers
mailing list