Skip to content

Conversation

@dlibenzi
Copy link
Collaborator

No description provided.

@dlibenzi
Copy link
Collaborator Author

Hold on on this one. I am waiting that you do the C++ test restructuring to add tests.
Or I can add tests before you do the restructuring.
LMK...

Copy link
Contributor

@asuhan asuhan left a comment

Choose a reason for hiding this comment

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

This looks pretty good, but it needs tests.

  1. Basic functionality: v = x.view(...), then x.add_(b)and v = x.view(...), then v.add_(b). In both cases x should be updated.
  2. View-of-view cases.
  3. Some more complex expressions involving view would be reassuring.

return id_generator->fetch_add(1);
}

std::pair<ir::NodePtr, bool> XLATensor::GetViewIrNode(View* view) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use a structure here instead of a pair.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense.

@asuhan
Copy link
Contributor

asuhan commented Jan 24, 2019

It's not much restructuring at all, you should be able to rebase just fine. But I'm sending the PR right now.

@dlibenzi
Copy link
Collaborator Author

Test added. Will add more complex tests later on.

Copy link
Contributor

@asuhan asuhan left a comment

Choose a reason for hiding this comment

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

Do you mind adding a Python version of the tests as well?

@dlibenzi
Copy link
Collaborator Author

Why should we add the Python versions?

@asuhan
Copy link
Contributor

asuhan commented Jan 24, 2019

We don't have to, we can move forward with just C++ tests. But we have to decide what should go in the Python tests then and how to deal with the debug mode not working for the C++ tests.

@dlibenzi dlibenzi merged commit 5a20136 into master Jan 25, 2019
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.

2 participants