Skip to content

Conversation

NiklasGustafsson
Copy link
Contributor

Fixed issue #621

@NiklasGustafsson NiklasGustafsson linked an issue Jun 8, 2022 that may be closed by this pull request
@NiklasGustafsson NiklasGustafsson changed the title Fixed bug #621 Adding Tensor.roll and torch.roll #621 Jun 8, 2022
/// Elements that are shifted beyond the last position are re-introduced at the first position.
/// If a dimension is not specified, the tensor will be flattened before rolling and then restored to the original shape.
/// </summary>
public Tensor roll((long,long) shifts, (long,long) dims)
Copy link
Member

Choose a reason for hiding this comment

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

,

nit: insert space.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to make the 'dims' argument a span, since it didn't let me default to 'null' Is there a solution to that? An empty span?

Copy link
Member

Choose a reason for hiding this comment

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

You can just pass empty span when having null dims. You can pass default value for the dims parameter.

/// Elements that are shifted beyond the last position are re-introduced at the first position.
/// If a dimension is not specified, the tensor will be flattened before rolling and then restored to the original shape.
/// </summary>
public Tensor roll((long,long) shifts, (long,long) dims)
Copy link
Member

Choose a reason for hiding this comment

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

,

nit: insert space.

/// Elements that are shifted beyond the last position are re-introduced at the first position.
/// If a dimension is not specified, the tensor will be flattened before rolling and then restored to the original shape.
/// </summary>
public Tensor roll(IEnumerable<long> shifts, IEnumerable<long>? dims = null)
Copy link
Member

Choose a reason for hiding this comment

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

IEnumerable

would it make sense more if you make this method takes either ReadOnlySpan which will avoid the array allocation in the callsite and also will avoid calling ToArray here too.

tarekgh
tarekgh previously approved these changes Jun 8, 2022
Copy link
Member

@tarekgh tarekgh left a comment

Choose a reason for hiding this comment

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

Added minor comments and a suggestion if you want to consider it. LGTM, otherwise.

public Tensor roll(long shifts, long? dims = null)
{
if (dims.HasValue) {
return roll(new long[] { shifts }, new long[] { dims.Value });
Copy link
Member

Choose a reason for hiding this comment

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

new long[]

Thanks for making Roll take a span. now you can convert all these long array allocations to stackalloc's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... When I tried changing to ReadOnlySpan for a few other APIs (torch.zeros, for example), that seems to not work with F#.

@dsyme -- this doesn't work if torch.zeros takes a ReadOnlySpan:

let mutable pe = torch.zeros([| maxLen; dmodel|])

Do I need to overload ROS and IEnumerable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to overload ROS and IEnumerable?

It seems I need to have roll(ReadOnlySpan,...) and roll(long[],...) as well as a private _roll(ReadOnlySpan...) with the actual implementation.

@NiklasGustafsson
Copy link
Contributor Author

I've been looking for other places to use ReadOnlySpan, but I will put that into separate PR.

@NiklasGustafsson NiklasGustafsson merged commit 7cfa039 into dotnet:main Jun 9, 2022
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.

torch.roll missing

2 participants