Skip to content

Implement an equivalent to numpy.roll - #1823

Merged
JohanMabille merged 1 commit into
xtensor-stack:masterfrom
serge-sans-paille:feature/roll
Dec 2, 2019
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

@serge-sans-pailleserge-sans-paille commented Nov 27, 2019

Copy link
Copy Markdown
Contributor

@serge-sans-paille

serge-sans-paille commented Nov 27, 2019

Copy link
Copy Markdown
ContributorAuthor

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

JohanMabille
JohanMabille reviewed Nov 28, 2019
View reviewed changes

@JohanMabilleJohanMabille 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.

Thank you very much!

Comment threadinclude/xtensor/xmanipulation.hpp Outdated
Comment threadinclude/xtensor/xmanipulation.hpp Outdated
Comment threadinclude/xtensor/xmanipulation.hpp Outdated
Comment threadinclude/xtensor/xmanipulation.hpp Outdated
Comment threadinclude/xtensor/xmanipulation.hpp Outdated
@tdegeustdegeus mentioned this pull request Nov 28, 2019
Closed
JohanMabille
JohanMabille reviewed Nov 28, 2019
View reviewed changes

@JohanMabilleJohanMabille 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.

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 threadinclude/xtensor/xmanipulation.hpp Outdated
}

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

@JohanMabilleJohanMabilleNov 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.

Formatting: opening curly brace should be on a new line

Comment threadinclude/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 {

@JohanMabilleJohanMabilleNov 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.

Same here

@serge-sans-paille

serge-sans-paille commented Nov 29, 2019

Copy link
Copy Markdown
ContributorAuthor

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
JohanMabille requested changes Dec 1, 2019
View reviewed changes

@JohanMabilleJohanMabille 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.

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

Comment threadinclude/xtensor/xmanipulation.hpp Outdated
Comment threadinclude/xtensor/xmanipulation.hpp
Comment threadinclude/xtensor/xmanipulation.hpp Outdated
@JohanMabille

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
serge-sans-pailleforce-pushed the feature/roll branch 3 times, most recently from fba18d3 to fb1ba45CompareDecember 1, 2019 21:38
@serge-sans-paille

serge-sans-paille commented Dec 2, 2019

Copy link
Copy Markdown
ContributorAuthor

@JohanMabille everything looks fine now?

@JohanMabille

JohanMabille commented Dec 2, 2019

Copy link
Copy Markdown
Member

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

@JohanMabille

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

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
JohanMabille merged commit 271ca8b into xtensor-stack:masterDec 2, 2019
This was referenced Dec 3, 2025
Draft
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JohanMabilleJohanMabilleJohanMabille 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

@serge-sans-paille@JohanMabille