Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement struct enumerator for Tensor<T> #46497

Merged
merged 13 commits into from
Mar 15, 2021
Merged

Implement struct enumerator for Tensor<T> #46497

merged 13 commits into from
Mar 15, 2021

Conversation

NewellClark
Copy link
Contributor

This fixes issue 28391.

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Jan 1, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes issue 28391.

Author: NewellClark
Assignees: -
Labels:

area-System.Net, new-api-needs-documentation
Milestone: -

@NewellClark
Copy link
Contributor Author

Oh dear. The bot applied the wrong tag. It should be area-System.Numerics.Tensors.

@ghost
Copy link

ghost commented Jan 1, 2021

Tagging subscribers to this area: @eiriktsarpalis, @pgovind
See info in area-owners.md if you want to be subscribed.

Issue Details

This fixes issue 28391.

Author: NewellClark
Assignees: -
Labels:

area-System.Numerics.Tensors, new-api-needs-documentation
Milestone: -

NewellClark
NewellClark commented Jan 2, 2021
View reviewed changes
Copy link
Contributor Author

@NewellClark NewellClark left a comment

Choose a reason for hiding this comment

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

Not sure why github is asking me to review my own pull request. I left the names the same to be consistent with the rest of the file, but applied the this = default suggested edit.

@NewellClark NewellClark closed this Jan 2, 2021
@NewellClark NewellClark reopened this Jan 2, 2021
@NewellClark
Copy link
Contributor Author

Okay, the runtime CI check failed with a "503 Service Not Available" result. I'm guessing this wasn't caused by the suggested changes. Gonna quickly close and reopen to try running them again.

@NewellClark NewellClark closed this Jan 2, 2021
@NewellClark NewellClark reopened this Jan 2, 2021
@NewellClark
Copy link
Contributor Author

Now one of the tests seems to have crashed. Been running for ~6 hours.

@NewellClark NewellClark closed this Jan 3, 2021
@NewellClark NewellClark reopened this Jan 3, 2021
@eiriktsarpalis
Copy link
Member

@eerhardt @danmosemsft do we need to put this through API review?

@eerhardt
Copy link
Member

eerhardt commented Jan 5, 2021

do we need to put this through API review?

This library hasn't been officially approved yet, nor has a stable version of the package been shipped. So my opinion is that we can iterate on this prototype library without bringing each change to API review. And then when we are happy with the whole shape, we bring the whole library to API review at once.

@NewellClark
Copy link
Contributor Author

Build just failed due to flaky test (issue #32805). Rerunning...

@NewellClark NewellClark reopened this Jan 15, 2021
@NewellClark NewellClark reopened this Jan 15, 2021
@jeffhandley
Copy link
Member

@stephentoub I believe this is ready for you re-review when you have time.

@NewellClark
Apply suggested changes
5dea73a 
-Make Reset and Dispose public
-Fix null ref bug with Reset
-Added tests for Reset and Dispose
@NewellClark
Merge remote-tracking branch 'dotnet/master' into enhancement-issue-2…
925dc7a 
…8391
Base automatically changed from master to main March 1, 2021 09:07
@stephentoub stephentoub merged commit 4a8ec1f into dotnet:main Mar 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 14, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SingleAccretion SingleAccretion SingleAccretion left review comments

@gfoidl gfoidl gfoidl left review comments

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Numerics.Tensors new-api-needs-documentation
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

Consider implementing struct Enumerator on Tensor<T>
10 participants
@NewellClark @Dotnet-GitSync-Bot @eiriktsarpalis @eerhardt @jeffhandley @stephentoub @gfoidl @SingleAccretion @karelz @danmoseley