Skip to content

[NFC/cleanup] Remove more entries of variables. #1875

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
Merged
merged 1 commit into from
Oct 16, 2018
Merged

[NFC/cleanup] Remove more entries of variables. #1875

merged 1 commit into from
Oct 16, 2018

Conversation

rdzhabarov
Copy link
Contributor

@rdzhabarov rdzhabarov commented Oct 16, 2018 •
edited
Loading

Description:
Removing references to variables when appropriate.

Testing:
unit tests

Documentation:
N/A

Related to #1334

@yinghai
Copy link
Contributor

yinghai commented Oct 16, 2018

Noob question: what's the reasoning of changing variable to input?

@rdzhabarov
Copy link
Contributor Author

rdzhabarov commented Oct 16, 2018 •
edited
Loading

That's a long discussion. See #1334 for details.
In short, there are no variables anymore. Now it's either constant or placeholder.

This PR is cleaning up what was left after migration.

jfix71
jfix71 reviewed Oct 16, 2018
View reviewed changes
Copy link
Contributor

@jfix71 jfix71 left a comment

Choose a reason for hiding this comment

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

I think I'm fine with updateInputs(), but I could see an argument for instead going with updateInputPlaceholders(), since Constants are also inputs to a Function, and the function signature takes an ArrayRef of Placeholders.

lib/Optimizer/Partition.cpp
@@ -96,7 +96,7 @@ NodeFunctionMap selectBasicBlockPartitions(Function *F) {

// If node has only one input, and that input has only one output, place it
// in the same partition.
auto *in = singleNonVariableInput(node);
auto *in = singleNonStorageInput(node);

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

@rdzhabarov
Copy link
Contributor Author

I like updateInputPlaceholders, more explicit to what's going on.

jfix71
jfix71 approved these changes Oct 16, 2018
View reviewed changes
Copy link
Contributor

@jfix71 jfix71 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@rdzhabarov rdzhabarov merged commit ac49da6 into pytorch:master Oct 16, 2018
@rdzhabarov rdzhabarov deleted the more_remove branch October 16, 2018 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bertmaher bertmaher bertmaher left review comments

@jfix71 jfix71 jfix71 approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rdzhabarov @yinghai @jfix71 @bertmaher @facebook-github-bot