Skip to content

Adding "digitize", "searchsorted", and "bin_items" - #2037

Merged
JohanMabille merged 4 commits into
xtensor-stack:masterfrom
tdegeus:digitize
May 5, 2020
Merged

JohanMabille merged 4 commits into
xtensor-stack:masterfrom
tdegeus:digitize

Conversation

@tdegeus

@tdegeus tdegeus commented Apr 21, 2020 •

Copy link
Copy Markdown
Member

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.

Description

@tdegeus tdegeus changed the title Adding "digitize" and "searchsorted" Adding "digitize", "searchsorted", and "bin_items" May 3, 2020
@tdegeus
tdegeus marked this pull request as ready for review May 3, 2020 12:22
@tdegeus
tdegeus requested a review from JohanMabille May 3, 2020 12:22
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xhistogram.hpp Resolved
Comment thread include/xtensor/xset_operation.hpp Resolved
@tdegeus

tdegeus commented May 5, 2020

Copy link
Copy Markdown
Member Author

Thanks @JohanMabille !

@tdegeus tdegeus closed this May 5, 2020
@tdegeus tdegeus reopened this May 5, 2020
@tdegeus

tdegeus commented May 5, 2020

Copy link
Copy Markdown
Member Author

I wanted to restart the Azure pipelines for all platforms (because it only shows OSX here), but not sure if it worked.

Anyway, normally things should be good-to-go.

@tdegeus tdegeus closed this May 5, 2020
@tdegeus tdegeus reopened this May 5, 2020
@JohanMabille

Copy link
Copy Markdown
Member

Looks like Azure didn't report everything to Github. However, everything run fine.

Comment on lines +177 to +190
if (right)
{
for (size_t i = 0; i < v.size(); ++i)
{
out(i) = std::lower_bound(a.cbegin(), a.cend(), v(i)) - a.cbegin();
}
}
else
{
for (size_t i = 0; i < v.size(); ++i)
{
out(i) = std::upper_bound(a.cbegin(), a.cend(), v(i)) - a.cbegin();
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey, this seems backwards to me. Is np.searchsorted(a, v, side="right") supposed to be identical to xt::searchsorted(a, v, true)? Because it doesn't... Considering the docstring of course... Maybe the docstring should be updated (to not break people's existing usages of this function).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants