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

Jump to bottom

2296: Fix erroneous less_equal usage in is_sorted calls #2471

Merged
merged 2 commits into from Jan 11, 2022
Merged

2296: Fix erroneous less_equal usage in is_sorted calls #2471

merged 2 commits into from Jan 11, 2022

Conversation

samuel-emrys
Copy link
Contributor

@samuel-emrys samuel-emrys commented Dec 27, 2021 •
edited

Description

  • Fix erroneous usage of the std::less_equal function as a Compare
    argument to is_sorted by replacing it with std::less. This will allow
    users to build and run with -D_GLIBCXX_DEBUG without causing a fatal
    error.
  • Update inline documentation to reflect change.

Fixes #2296

Checklist

  • The title and commit message(s) are descriptive.
  • Small commits made to fix your PR have been squashed to avoid history pollution.
  • Tests have been added for new features or bug fixes.
  • [] API of new functions and classes are documented.

I'm not sure how this can be tested within the existing unit test suite as it requires different compilation options to reproduce. Compiling the following exemplar program with -D_GLIBCXX_DEBUG would test this sufficiently:

auto main() -> int {
    xt::xarray<int> v{1, 5, 9, 3, 2, 9, 7};
    auto xt_sum = xt::sum(v)();
    return EXIT_SUCCESS;
}

This is a working demonstration of the error on godbolt.

@samuel-emrys
2296: Fix erroneous less_equal usage in is_sorted calls
fb0c66e 
* Fix erroneous usage of the std::less_equal function as a Compare
  argument to is_sorted by replacing it with std::less. This will allow
  users to build and run with -D_GLIBCXX_DEBUG without causing a fatal
  error.
* Update inline documentation to reflect change.

Fixes xtensor-stack#2296
@samuel-emrys
2296: Add test for duplicates in reduction indices
bce3e95 
* Add test for duplicates in reduction indices. This was necessary
  because changing the binary predicate from <= to < when comparing axes
  indices no longer excludes duplicate values when passed to
  std::is_sorted. This allows both duplicates and unsorted axis indices
  to be identified and filtered appropriately.
* Separate tests for sorted and duplicate indices for readability and
  improved clarity in exception messages.
* Add unit tests to ensure all combinations of double axis indices are
  handled appropriately.
* Update numpy documentation to reflect correct numpy axis specification
  syntax as a tuple instead of a list. According to the numpy documentation
  "If axis is a tuple of ints ...". This is important because specification
  of axis indices as a list yields an error. The list type has been
  updated to tuple for accuracy. For confirmation of this, refer to:
  https://numpy.org/doc/stable/reference/generated/numpy.sum.html#numpy.sum

Fixes xtensor-stack#2296
@samuel-emrys
Copy link
Contributor Author

@samuel-emrys samuel-emrys commented Dec 28, 2021

Can anybody provide any guidance on the failing CI tests for Linux_1_gcc_7_openmp? The unit test in question isn't one that seems to be affected on any other platform - is this a spurious failure or an actual error?

@ojschumann
Copy link

@ojschumann ojschumann commented Jan 4, 2022

Hi, I had the same problem recently (and run into it again in xtensor. Thas why I found this PR)

The two calls to std::is_sorted and std::adjacent_find might be combined:

if (!std::is_sorted(axes.cbegin(), axes.cend(), std::less_equal<>())) is equivalent to
if (std::adjacent_find(axes.cbegin(), axes.cend(), std::greater_equal<>()) != axes.cend())

@JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Jan 5, 2022

@samuel-emrys sorry for the late reply, this is indeed a flaky error, I've restarted the job.

@tdegeus
Copy link
Member

@tdegeus tdegeus commented Jan 9, 2022

LGTM, thanks!

@JohanMabille JohanMabille merged commit 01ccbc0 into xtensor-stack:master Jan 11, 2022
20 checks passed
@JohanMabille
Copy link
Member

@JohanMabille JohanMabille commented Jan 11, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Issue when compiling with -D_GLIBCXX_DEBUG
4 participants
@samuel-emrys @ojschumann @JohanMabille @tdegeus