FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement an equivalent to numpy.roll by serge-sans-paille · Pull Request #1823 · xtensor-stack/xtensor · GitHub

  1. xtensor-stack
  2. xtensor

Repository navigation

Implement an equivalent to numpy.roll - #1823

Merged
JohanMabille merged 1 commit into
xtensor-stack:masterfrom
serge-sans-paille:feature/roll
Dec 2, 2019
ConversationCommits (1)ChecksFiles changed
Merged

Implement an equivalent to numpy.roll#1823
JohanMabille merged 1 commit into
xtensor-stack:masterfrom
serge-sans-paille:feature/roll

Conversation

serge-sans-paille commented Nov 27, 2019

Copy link
Copy Markdown
Contributor

Fix #1766

serge-sans-paille commented Nov 27, 2019

Copy link
Copy Markdown
Contributor Author

It's just a WIP, when there's no axis given. Feel free to comment, I'm new to xtensor's internal.

JohanMabille reviewed Nov 28, 2019
View reviewed changes

JohanMabille left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thank you very much!

Comment thread include/xtensor/xmanipulation.hpp Outdated
Comment thread include/xtensor/xmanipulation.hpp Outdated
Comment thread include/xtensor/xmanipulation.hpp Outdated
Comment thread include/xtensor/xmanipulation.hpp Outdated
Comment thread include/xtensor/xmanipulation.hpp Outdated
tdegeus mentioned this pull request Nov 28, 2019
Closed
JohanMabille reviewed Nov 28, 2019
View reviewed changes

JohanMabille left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I didn't notice the first time (maybe you switched to another machine), it seems that the indentation in the roll function is 2 spaces instead of 4 (like it is in the cpp for testing or other functions in the header).

Once the nformatting is fixed, do you want this PR to be merged and to implement the overload taking an axis parameter in another PR? Or do you prefer to implement everything in a single PR?

Comment thread include/xtensor/xmanipulation.hpp Outdated
}

template<class E>
inline auto roll(E&& e, std::ptrdiff_t shift) {

JohanMabille Nov 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Formatting: opening curly brace should be on a new line

Comment thread include/xtensor/xmanipulation.hpp Outdated
std::copy(eflat_view.begin(), eflat_view.begin() + shift,
std::copy(eflat_view.begin() + shift, eflat_view.end(), cpyflat_view.begin()));
}
else {

JohanMabille Nov 28, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Same here

serge-sans-paille commented Nov 29, 2019

Copy link
Copy Markdown
Contributor Author

Validation fixed and comments taken into account!

Once the formatting is fixed, do you want this PR to be merged

I'll implement the axis stuff first, if that's okay with you.

JohanMabille requested changes Dec 1, 2019
View reviewed changes

JohanMabille left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think you still have a mix of 2 spaces and 4 spaces indentation.

Comment thread include/xtensor/xmanipulation.hpp Outdated
Comment thread include/xtensor/xmanipulation.hpp
Comment thread include/xtensor/xmanipulation.hpp Outdated

JohanMabille commented Dec 1, 2019

Copy link
Copy Markdown
Member

Also regarding travis-ci:

  • the error of the build with ENABLE_OPENMP is unrelated to this PR.
  • the error of the build with COLUMN_MAJOR_LAYOUT is related to this PR.

serge-sans-paille force-pushed the feature/roll branch 3 times, most recently from fba18d3 to fb1ba45 Compare December 1, 2019 21:38

serge-sans-paille commented Dec 2, 2019

Copy link
Copy Markdown
Contributor Author

@JohanMabille everything looks fine now?

JohanMabille commented Dec 2, 2019

Copy link
Copy Markdown
Member

Almost, it seems that the indentation is still 2 spaces instead of 4.

JohanMabille commented Dec 2, 2019

Copy link
Copy Markdown
Member

Also notice that the error on travisci will disappear when the PR is merged (or if it is rebased on master before merging).

JohanMabille commented Dec 2, 2019

Copy link
Copy Markdown
Member

There are actually other parts of the library with 2 spaces indentation, let's merge as is, I will do another pass to clean everything at once.

JohanMabille merged commit 271ca8b into xtensor-stack:master Dec 2, 2019
This was referenced Dec 3, 2025
Draft
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

JohanMabille JohanMabille approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

No equivalent to numpy.roll in xtensor

2 participants

Back | FazBrowse Home | New Git URL