Skip to content

Conversation

@jramapuram
Copy link
Member

Upstream has:

  • rows()
  • cols()

Adding these to the wrapper.

src/index.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

@jramapuram We do not need to retrieve dimensions, just doing &[Seq::new(first, last, 1), Seq::default()] would be enough for this. This way one less ffi call in terms of input.dims().

Seq::default is the equivalent of af_span in C API of ArrayFire that spans all elements along that dimension.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will refactor to span.

What about assertion of ranges? I.e. making sure first & last are contained within the max_rows?

Copy link
Member

Choose a reason for hiding this comment

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

Should be fine i think, index call will return appropriate error code if something is wrong. I don't think we should duplicate error checks in rust wrapper unless absolutely required.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@9prady9
Copy link
Member

9prady9 commented Sep 20, 2015

@jramapuram Can also please add slices wrapper, it would be exactly same except that the Seq using the parameters would be along third dimension.

@jramapuram
Copy link
Member Author

Cool will refactor to seq default & add span

@jramapuram
Copy link
Member Author

@9prady9 : Refactored and added setters. Please review.
Also, cleaned up the seq_gen API.

@9prady9
Copy link
Member

9prady9 commented Sep 21, 2015

👍

9prady9 added a commit that referenced this pull request Sep 21, 2015
@9prady9 9prady9 merged commit 11a47f3 into master Sep 21, 2015
@9prady9 9prady9 deleted the feature/add_rows_cols branch September 21, 2015 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants