Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is ITK_USE_FLOAT_SPACE_PRECISION obsolete? Build errors when = YES. #4802

Closed
seanm opened this issue Aug 14, 2024 · 14 comments
Closed

Is ITK_USE_FLOAT_SPACE_PRECISION obsolete? Build errors when = YES. #4802

seanm opened this issue Aug 14, 2024 · 14 comments
Assignees
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances

Comments

@seanm
Copy link
Contributor

seanm commented Aug 14, 2024

I've been building ITK with various build option permutations (to try and shake out the last -Wextra-semi warnings) and it seems to me that ITK does not build with ITK_USE_FLOAT_SPACE_PRECISION = YES.

ex:

Modules/Core/ImageFunction/test/itkVectorInterpolateImageFunctionTest.cxx:223:14: error: no matching conversion for functional-style cast from 'itk::SpacePrecisionType[3]' (aka 'float[3]') to 'ContinuousIndexType' (aka 'ContinuousIndex<double, Self::ImageDimension>')
    cindex = ContinuousIndexType(darray);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~

and

Modules/Filtering/ImageGrid/test/itkBinShrinkImageFilterTest2.cxx:45:12: error: no matching function for call to 'FloatAlmostEqual'
      if (!itk::Math::FloatAlmostEqual(pt[i], it.Get()[i]))
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~

and others too.

Seems like no one is using this if it doesn't even build?

@seanm seanm added the type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances label Aug 14, 2024
@dzenanz
Copy link
Member

dzenanz commented Aug 14, 2024

ITK_USE_FLOAT_SPACE_PRECISION tells whether the default precision should be float instead of double. I am fine with removal of this option, as long as float precision can be achieved by providing it as a (template) parameter instead of the default double to relevant classes (interpolators, PointTypes, etc).

@dzenanz
Copy link
Member

dzenanz commented Aug 14, 2024

Thorough testing of float space precision is hard, and having a build option to make float the default is an excellent way to test it. That's why I would prefer if the build with this option gets fixed, instead of the option being removed.

@dzenanz
Copy link
Member

dzenanz commented Aug 14, 2024

How many compile errors are there when ITK_USE_FLOAT_SPACE_PRECISION = YES?

@seanm
Copy link
Contributor Author

seanm commented Aug 14, 2024

I count 7, but it aborts building at 74%.

@dzenanz dzenanz self-assigned this Aug 15, 2024
@dzenanz
Copy link
Member

dzenanz commented Aug 15, 2024

I kicked off a build with ITK_USE_FLOAT_SPACE_PRECISION ON, and I plan to tackle compile errors over the following days.

@seanm
Copy link
Contributor Author

seanm commented Aug 15, 2024

If you decide to fix the errors, you might want to do it in the release branch too.

@dzenanz
Copy link
Member

dzenanz commented Dec 13, 2024

This broke again with recent changes. Here is an output for one project:

------ Build started: Project: ITKImageGridGTestDriver, Configuration: Debug x64 ------
itkResampleImageFilterGTest.cxx
C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28): error C2664: 'itk::Point<itk::ImageBase<2>::PointValueType,2> itk::ImageBase<2>::TransformContinuousIndexToPhysicalPoint<itk::SpacePrecisionType,itk::SpacePrecisionType>(const itk::ContinuousIndex<TCoordinate,2> &) const': cannot convert argument 1 from 'itk::ImageAlgorithm::EnlargeRegionOverBox::ContinuousInputIndexType' to 'const itk::ContinuousIndex<TCoordinate,2> &'
C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28): error C2664:         with
C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28): error C2664:         [
C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28): error C2664:             TCoordinate=float
C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28): error C2664:         ]
(compiling source file '../../../../../ITK-git/Modules/Filtering/ImageGrid/test/itkResampleImageFilterGTest.cxx')
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(245,9):
    Reason: cannot convert from 'itk::ImageAlgorithm::EnlargeRegionOverBox::ContinuousInputIndexType' to 'const itk::ContinuousIndex<TCoordinate,2>'
        with
        [
            TCoordinate=float
        ]
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(245,9):
    No user-defined-conversion operator available that can perform this conversion, or the operator cannot be called
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageBase.h(575,3):
    see declaration of 'itk::ImageBase<2>::TransformContinuousIndexToPhysicalPoint'
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageBase.h(555,3):
    could be 'void itk::ImageBase<2>::TransformContinuousIndexToPhysicalPoint(const itk::ContinuousIndex<TIndexRep,2> &,itk::Point<TCoordinate,2> &) const'
        C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28):
        'void itk::ImageBase<2>::TransformContinuousIndexToPhysicalPoint(const itk::ContinuousIndex<TIndexRep,2> &,itk::Point<TCoordinate,2> &) const': expects 2 arguments - 1 provided
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28):
    while trying to match the argument list '(itk::ImageAlgorithm::EnlargeRegionOverBox::ContinuousInputIndexType)'
    C:\Dev\ITK-git\Modules\Core\Common\include\itkImageAlgorithm.hxx(244,28):
    the template instantiation context (the oldest one first) is
        C:\Dev\ITK-git\Modules\Filtering\ImageGrid\test\itkResampleImageFilterGTest.cxx(117,88):
        see reference to class template instantiation 'itk::ResampleImageFilter<itk::Image<int,2>,itk::Image<int,2>,float,float>' being compiled
        C:\Dev\ITK-git\Modules\Filtering\ImageGrid\include\itkResampleImageFilter.hxx(527,1):
        while compiling class template member function 'void itk::ResampleImageFilter<itk::Image<int,2>,itk::Image<int,2>,float,float>::GenerateInputRequestedRegion(void)'
        C:\Dev\ITK-git\Modules\Filtering\ImageGrid\include\itkResampleImageFilter.hxx(559,23):
        see reference to function template instantiation 'itk::ImageRegion<2> itk::ImageAlgorithm::EnlargeRegionOverBox<itk::Image<int,2>,itk::Image<int,2>,itk::Transform<TTransformPrecisionType,2,2>>(const itk::ImageRegion<2> &,const InputImageType *,const OutputImageType *,const TransformType *)' being compiled
        with
        [
            TTransformPrecisionType=float,
            InputImageType=itk::Image<int,2>,
            OutputImageType=itk::Image<int,2>,
            TransformType=itk::Transform<float,2,2>
        ]
itkTileImageFilterGTest.cxx
Generating Code...
Done building project "ITKImageGridGTestDriver.vcxproj" -- FAILED.

@dzenanz
Copy link
Member

dzenanz commented Dec 13, 2024

I now turned on ITK_USE_FLOAT_SPACE_PRECISION in Windows11-VS22x64-RelWithDebInfo-Favorite-Remotes nightly test build.

@seanm
Copy link
Contributor Author

seanm commented Dec 17, 2024

I now turned on ITK_USE_FLOAT_SPACE_PRECISION in Windows11-VS22x64-RelWithDebInfo-Favorite-Remotes nightly test build.

I hear Bill Lorensen saying 'untested code is broken code' in my head :)

@seanm
Copy link
Contributor Author

seanm commented Dec 17, 2024

a thought: Is this broken in the 5.4.1 tag?

@dzenanz
Copy link
Member

dzenanz commented Dec 17, 2024

Probably not. This got broken in master.

@hjmjohnson
Copy link
Member

Can you give some context for why this is needed? I'm trying to think of a usecase where this has benefit.

Is there someone who uses this feature? v6.0.0 could be a good time to remove the feature.

PS: I'm building with this flag on with wrapping turned on.

@hjmjohnson
Copy link
Member

addressed in

commit c9b342eefbd95a2506c8e816e33c11976ea0de62 (remove-alias)
Author: Hans Johnson <[email protected]>
Date:   2024-12-17 20:11:43 -0600

    COMP: Fix building with ITK_USE_FLOAT_SPACE_PRECISION=ON
    
    Need to match precision for types used to allow building.
    In a few cases double precision was assumed for values
    related to spacing precision.

commit cad42971d5d65d17b56359b06ab9924696cfdce1
Author: Hans Johnson <[email protected]>
Date:   2024-12-17 20:09:50 -0600

    COMP: Prohibit both ITK_USE_FLOAT_SPACE_PRECISION & ITK_WRAP_PYTHON
    
    Using float space precision is not supported with wrapping python.
    The python wrapping assumes a limited number of types, and those
    types would need to be aliased for supporting float space
    continuous indexes (and other types)

@dzenanz
Copy link
Member

dzenanz commented Jan 23, 2025

I also turned on ITK_USE_FLOAT_SPACE_PRECISION in nightly build named Windows11-VS22x64-RelWithDebInfo-Favorite-Remotes. If some change breaks it, we should know soon thereafter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants