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.

Sign up for GitHub

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

Jump to bottom

Fix performance issue with nanstd and nanvar #2308

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix performance issue with nanstd and nanvar #2308

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions include/xtensor/xmath.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2671,8 +2671,8 @@ namespace detail {
XTL_REQUIRES(is_reducer_options<EVS>)>
inline auto nanvar(E&& e, EVS es = EVS())
{
decltype(auto) sc = detail::shared_forward<E>(e);
return nanmean<T>(square(sc - nanmean<T>(sc)), es);
auto cached_mean = nanmean<T>(e, es)();
return nanmean<T>(square(std::forward<E>(e) - std::move(cached_mean)), es);
}

template <class T = void, class E, class EVS = DEFAULT_STRATEGY_REDUCERS,
Expand Down Expand Up @@ -2709,7 +2709,8 @@ namespace detail {
// note: forcing copy of first axes argument -- is there a better solution?
auto axes_copy = axes;
using result_type = typename std::conditional_t<std::is_same<T, void>::value, double, T>;
auto inner_mean = nanmean<result_type>(sc, std::move(axes_copy));
// always eval to prevent repeated evaluations in the next calls
auto inner_mean = eval(nanmean<result_type>(sc, std::move(axes_copy)));

// fake keep_dims = 1
auto keep_dim_shape = e.shape();
Expand All @@ -2718,7 +2719,9 @@ namespace detail {
keep_dim_shape[el] = 1;
}
auto mrv = reshape_view<XTENSOR_DEFAULT_LAYOUT>(std::move(inner_mean), std::move(keep_dim_shape));
return nanmean<result_type>(square(cast<result_type>(sc) - std::move(mrv)), std::forward<X>(axes), es);
// note: otherwise the result is wrong with 'immediate' evaluation strategy
auto sc_shifted = eval(cast<result_type>(sc) - std::move(mrv));
return nanmean<result_type>(square(sc_shifted), std::forward<X>(axes), es);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am completely lost and I summarize my findings here:

Test code:

xt::xarray<double> aN = {{ nanv, nanv, 123, 3 }, { 1, 2, nanv, 3 }, { 1, 1, nanv, 3 }};

  xt::xarray<double> ret2 = xt::nanvar(aN, {1});
  std::cout << "Lazy: " << ret2 << std::endl;

  xt::xarray<double> ret = xt::nanvar(aN, {1}, xt::evaluation_strategy::immediate);
  std::cout << "Immediate: " << ret << std::endl;
  • First case:
auto sc_shifted = eval(cast<result_type>(sc) - std::move(mrv));
return nanmean<result_type>(square(sc_shifted), std::forward<X>(axes), es);

Result with gcc8:

Lazy: { 2400.      ,     0.666667,     0.888889}
Immediate: { 3600.      ,     0.666667,     0.888889}

Result with Clang7: segfault

	je     0x4198a0                   ; <xt::xreducer_stepper<xt::xreducer_functors<xt::detail::nan_plus, xt::const_value<double>, xt::detail::nan_plus>, xt::xshared_expression<xt::xfunction<xt::detail::lambda_adapt<xt::square_fct>, xt::xarray_container<xt::uvector<double, std::allocator<double> >, (xt::layout_type)1, xt::svector<unsigned long, 4ul, std::allocator<unsigned long>, true>, xt::xtensor_expression_tag> const&> >, std::array<unsigned long, 1ul>, xt::reducer_options<double, std::tuple<xt::evaluation_strategy::lazy_type> > >::aggregate_impl(unsigned long, std::integral_constant<bool, false>) const+784>
	shl    $0x3,%rcx
	neg    %rax
	mov    %rdi,%rsi
	nopw   0x0(%rax,%rax,1)
	vmovsd (%rsi),%xmm0
  • Second case:
return nanmean<result_type>(square(cast<result_type>(sc) - std::move(mrv)), std::forward<X>(axes), es);

Result with gcc8:

Lazy: { 3600.      ,     0.666667,     0.888889}
Immediate: { 7365.388889,     4.666667,    12.      }

Result with clang7:

Lazy: { 3600.      ,     0.666667,     0.888889}
Immediate: { 7365.388889,    12.      ,   363.      }

@JohanMabille @tdegeus Can you give some hint? Thank you!

}

/**
Expand Down