Skip to content

Switch half_float::half to Eigen::half#3224

Merged
Lestropie merged 2 commits intodevfrom
eigen_half
Oct 25, 2025
Merged

Switch half_float::half to Eigen::half#3224
Lestropie merged 2 commits intodevfrom
eigen_half

Conversation

@Lestropie
Copy link
Member

Closes #3180.

I had an initial unsuccessful go at this; pushing draft as I need to switch to other tasks and don't want it to disappear.

Initially looked like it would be a fairly trivial switchout, but was having trouble getting template <class ImageType> class Value to work. Here's my current compilation issue:

FAILED: cpp/core/CMakeFiles/mrtrix-core.dir/image.cpp.o 
/usr/bin/ccache /usr/bin/c++ -DMRTRIX_BASE_VERSION=\"3.0.7\" -DMRTRIX_BUILD_TYPE=\"Debug\" -DMRTRIX_HAVE_LGAMMA_R -DMRTRIX_PNG_SUPPORT -Dmrtrix_core_EXPORTS -I/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core -I/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/eigen3-src -I/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/json-src/include -I/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/nifti1-src -I/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/nifti2-src -g -fPIC -std=gnu++17 -Winvalid-pch -include /home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/cpp/core/CMakeFiles/mrtrix-core.dir/cmake_pch.hxx -MD -MT cpp/core/CMakeFiles/mrtrix-core.dir/image.cpp.o -MF cpp/core/CMakeFiles/mrtrix-core.dir/image.cpp.o.d -o cpp/core/CMakeFiles/mrtrix-core.dir/image.cpp.o -c /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image.cpp
In file included from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/loop.h:20,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/copy.h:19,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image.h:25,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image.cpp:16:
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image_helpers.h: In instantiation of ‘MR::Helper::Value<ImageType>::value_type MR::Helper::Value<ImageType>::operator=(MR::Helper::Value<OtherImageType>&&) [with OtherImageType = MR::{anonymous}::TmpImage<Eigen::half>; ImageType = MR::Image<Eigen::half>; MR::Helper::Value<ImageType>::value_type = Eigen::half]’:
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_copy.h:29:17:   required from ‘void MR::{anonymous}::__copy_func::operator()(InputImageType&, OutputImageType&) const [with InputImageType = MR::{anonymous}::TmpImage<Eigen::half>; OutputImageType = MR::Image<Eigen::half>]’
/usr/include/c++/11/bits/invoke.h:61:36:   required from ‘constexpr _Res std::__invoke_impl(std::__invoke_other, _Fn&&, _Args&& ...) [with _Res = void; _Fn = MR::{anonymous}::__copy_func&; _Args = {MR::{anonymous}::TmpImage<Eigen::half>&, MR::Image<Eigen::half>&}]’
/usr/include/c++/11/bits/invoke.h:96:40:   required from ‘constexpr typename std::__invoke_result<_Functor, _ArgTypes>::type std::__invoke(_Callable&&, _Args&& ...) [with _Callable = MR::{anonymous}::__copy_func&; _Args = {MR::{anonymous}::TmpImage<Eigen::half>&, MR::Image<Eigen::half>&}; typename std::__invoke_result<_Functor, _ArgTypes>::type = void]’
/usr/include/c++/11/tuple:1854:27:   required from ‘constexpr decltype(auto) std::__apply_impl(_Fn&&, _Tuple&&, std::index_sequence<_Idx ...>) [with _Fn = MR::{anonymous}::__copy_func&; _Tuple = std::tuple<MR::{anonymous}::TmpImage<Eigen::half>, MR::Image<Eigen::half> >&; long unsigned int ..._Idx = {0, 1}; std::index_sequence<_Idx ...> = std::integer_sequence<long unsigned int, 0, 1>]’
/usr/include/c++/11/tuple:1865:31:   required from ‘constexpr decltype(auto) std::apply(_Fn&&, _Tuple&&) [with _Fn = MR::{anonymous}::__copy_func&; _Tuple = std::tuple<MR::{anonymous}::TmpImage<Eigen::half>, MR::Image<Eigen::half> >&]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_loop.h:278:17:   required from ‘void MR::{anonymous}::ThreadedLoopRunInner<N, Functor, ImageType>::operator()(const MR::Iterator&) [with int N = 2; Functor = MR::{anonymous}::__copy_func; ImageType = {MR::{anonymous}::TmpImage<Eigen::half>, MR::Image<Eigen::half>}]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_loop.h:315:16:   required from ‘void MR::{anonymous}::ThreadedLoopRunOuter<OuterLoopType>::run_outer(Functor&&) [with Functor = MR::{anonymous}::ThreadedLoopRunInner<2, MR::{anonymous}::__copy_func, MR::{anonymous}::TmpImage<Eigen::half>, MR::Image<Eigen::half> >&; OuterLoopType = MR::LoopAlongDynamicAxesProgress]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_loop.h:369:14:   required from ‘void MR::{anonymous}::ThreadedLoopRunOuter<OuterLoopType>::run(Functor&&, ImageType&& ...) [with Functor = MR::{anonymous}::__copy_func; ImageType = {MR::{anonymous}::TmpImage<Eigen::half>&, MR::Image<Eigen::half>&}; OuterLoopType = MR::LoopAlongDynamicAxesProgress]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_copy.h:70:76:   required from ‘void MR::threaded_copy_with_progress_message(const string&, InputImageType&, OutputImageType&, size_t, size_t, size_t) [with InputImageType = MR::{anonymous}::TmpImage<Eigen::half>; OutputImageType = MR::Image<Eigen::half>; std::string = std::__cxx11::basic_string<char>; size_t = long unsigned int]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image.h:352:44:   required from ‘MR::Image<ValueType>::~Image() [with ValueType = Eigen::half]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image.cpp:28:37:   required from here
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image_helpers.h:501:40: error: use of deleted function ‘MR::Helper::Value<ImageType>::Value(const MR::Helper::Value<ImageType>&) [with ImageType = MR::{anonymous}::TmpImage<Eigen::half>]’
  501 |     return set(static_cast<value_type>(static_cast<typename OtherImageType::value_type>(V)));
      |                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image_helpers.h:494:3: note: declared here
  494 |   Value(const Value &) = delete;
      |   ^~~~~
In file included from /home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/eigen3-src/Eigen/Core:176,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/eigen3-src/Eigen/Geometry:11,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/types.h:71,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/exception.h:19,
                 from /home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/cpp/core/CMakeFiles/mrtrix-core.dir/cmake_pch.hxx:5,
                 from <command-line>:
/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/eigen3-src/Eigen/src/Core/arch/Default/Half.h:181:37: note:   initializing argument 1 of ‘Eigen::half::half(T) [with T = MR::Helper::Value<MR::{anonymous}::TmpImage<Eigen::half> >]’
  181 |   explicit EIGEN_DEVICE_FUNC half(T val)
      |                                   ~~^~~
[14/310] Building CXX object cpp/core/CMakeFiles/mrtrix-core.dir/math/average_space.cpp.o
ninja: build stopped: subcommand failed.

, where the relevant code in aa0d8b6 is:

  template <typename OtherImageType> FORCE_INLINE value_type operator=(Value<OtherImageType> &&V) {
    return set(static_cast<value_type>(static_cast<typename OtherImageType::value_type>(V)));
  }

So despite an explicit static_cast<>, it's still trying to construct an Eigen::half from a Value<ImageType>. Presumably it's due to the use of an rvalue reference, which I have almost zero experience with; I couldn't get std::remove_reference() to work for me as any attempt would involve invoking the deleted Value<ImageType> copy-constructor. If there's no trivial way around it, could define a class that inherits from Eigen::half and provides the requisite interface. Content to do that effort byt will first await if @daljit46 or @jdtournier can immediately see a fix.

@Lestropie Lestropie self-assigned this Oct 20, 2025
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy made some suggestions

template <typename OtherType> FORCE_INLINE value_type operator=(Value<OtherType> &&V) {
return set(typename OtherType::value_type(V));
template <typename OtherImageType> FORCE_INLINE value_type operator=(Value<OtherImageType> &&V) {
return set(static_cast<value_type>(static_cast<typename OtherImageType::value_type>(V)));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning: call to deleted constructor of 'MR::Helper::Value<MR::(anonymous namespace)::TmpImageEigen::half>' [clang-diagnostic-error]

    return set(static_cast<value_type>(static_cast<typename OtherImageType::value_type>(V)));
                                                                                        ^
Additional context

cpp/core/algo/threaded_copy.h:28: in instantiation of function template specialization 'MR::Helper::Value<MR::ImageEigen::half>::operator=<MR::(anonymous namespace)::TmpImageEigen::half>' requested here

    out.value() = in.value();
                ^

/usr/include/c++/13/bits/invoke.h:60: in instantiation of function template specialization 'MR::(anonymous namespace)::__copy_func::operator()<MR::(anonymous namespace)::TmpImageEigen::half, MR::ImageEigen::half>' requested here

    { return std::forward<_Fn>(__f)(std::forward<_Args>(__args)...); }
             ^

cpp/core/algo/threaded_loop.h:314: in instantiation of member function 'MR::(anonymous namespace)::ThreadedLoopRunInner<2, MR::(anonymous namespace)::__copy_func, MR::(anonymous namespace)::TmpImageEigen::half, MR::ImageEigen::half>::operator()' requested here

         ^

cpp/core/algo/threaded_loop.h:368: in instantiation of function template specialization 'MR::(anonymous namespace)::ThreadedLoopRunOuterMR::LoopAlongDynamicAxesProgress::run_outer<MR::(anonymous namespace)::ThreadedLoopRunInner<2, MR::(anonymous namespace)::__copy_func, MR::(anonymous namespace)::TmpImageEigen::half, MR::ImageEigen::half> &>' requested here

     ^

cpp/core/algo/threaded_copy.h:69: in instantiation of function template specialization 'MR::(anonymous namespace)::ThreadedLoopRunOuterMR::LoopAlongDynamicAxesProgress::run<MR::(anonymous namespace)::__copy_func, MR::(anonymous namespace)::TmpImageEigen::half &, MR::ImageEigen::half &>' requested here

  ThreadedLoop(message, source, from_axis, to_axis, num_axes_in_thread).run(__copy_func(), source, destination);
                                                                        ^

cpp/core/image.h:351: in instantiation of function template specialization 'MR::threaded_copy_with_progress_message<MR::(anonymous namespace)::TmpImageEigen::half, MR::ImageEigen::half>' requested here

        threaded_copy_with_progress_message("writing back direct IO buffer for \"" + name() + "\"", src, dest);
        ^

cpp/core/image_helpers.h:493: 'Value' has been explicitly marked deleted here

  Value(const Value &) = delete;
  ^

_deps/eigen3-src/Eigen/src/Core/arch/Default/Half.h:180: passing argument to parameter 'val' here

  explicit EIGEN_DEVICE_FUNC half(T val)
                                    ^

@Lestropie
Copy link
Member Author

I had thought that:

template <typename OtherImageType> FORCE_INLINE value_type operator=(Value<OtherImageType> &&V) {
    return set(static_cast<value_type>(static_cast<typename OtherImageType::value_type>(std::forward<Value<OtherImageType>>(V))));
  }

might get it, but it seems Eigen doesn't like copy-constructing half-floats from half-floats:

FAILED: cpp/core/CMakeFiles/mrtrix-core.dir/image.cpp.o 
...
src/Eigen/src/Core/arch/Default/Half.h: In instantiation of ‘Eigen::half::half(T) [with T = MR::Helper::Value<MR::{anonymous}::TmpImage<Eigen::half> >]’:
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/image_helpers.h:501:40:   required from ‘MR::Helper::Value<ImageType>::value_type MR::Helper::Value<ImageType>::operator=(MR::Helper::Value<OtherImageType>&&) [with OtherImageType = MR::{anonymous}::TmpImage<Eigen::half>; ImageType = MR::Image<Eigen::half>; MR::Helper::Value<ImageType>::value_type = Eigen::half]’
/home/unimelb.edu.au/robertes/src/mrtrix3/cpp/core/algo/threaded_copy.h:29:17:   required from ‘void MR::{anonymous}::__copy_func::operator()(InputImageType&, OutputImageType&) const [with InputImageType = MR::{anonymous}::TmpImage<Eigen::half>; OutputImageType = MR::Image<Eigen::half>]’
...
/home/unimelb.edu.au/robertes/src/mrtrix3/build_debug/_deps/eigen3-src/Eigen/src/Core/arch/Default/Half.h:182:60: error: invalid ‘static_cast’ from type ‘MR::Helper::Value<MR::{anonymous}::TmpImage<Eigen::half> >’ to type ‘float’
  182 |       : half_impl::half_base(half_impl::float_to_half_rtne(static_cast<float>(val))) {}
      |                                                            ^~~~~~~~~~~~~~~~~~~~~~~
ninja: build stopped: subcommand failed.

The Eigen::half copy-constructors are wrapped in #ifdef's, presumably those are absent from my machine leading to attempted use of the template constructor. Also presuming there's a good reason for the Eigen code being the way that it is. A few possibilities of how to possibly get around this, but deferring for now.

@daljit46
Copy link
Member

This is quite tricky C++. The statement static_cast<typename OtherImageType::value_type>(V) is effectively invoking Eigen::half(V) which takes V by value (this is because named rvalues variables inside a function are actually lvalues according to C++ spec, see this). However, you can just force the compiler to instead use the conversion operator in the Value class (which will construct the scalar value by using image.get_value()) by doing:

  template <typename OtherImageType> FORCE_INLINE value_type operator=(Value<OtherImageType> &&V) {
    // Use conversion operator explicitly to get an Eigen::half
    const auto rhs = V.operator typename OtherImageType::value_type();
    return set(static_cast<value_type>(rhs));
  }

@Lestropie Lestropie marked this pull request as ready for review October 24, 2025 23:36
@Lestropie
Copy link
Member Author

Personally I'd like to proceed with this: the prior half dependency often fails to download during configuration, the code changes are minimal, and while C++23 has FP16 we've still got a lot of refactoring to reasonably make use of C++17 let alone 23.

@github-actions
Copy link

clang-tidy review says "All clean, LGTM! 👍"

@Lestropie Lestropie merged commit 0bcaa3c into dev Oct 25, 2025
6 checks passed
@Lestropie Lestropie deleted the eigen_half branch October 25, 2025 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants