Skip to content

Conversation

@reyoung
Copy link
Collaborator

@reyoung reyoung commented Oct 10, 2017

No description provided.

@reyoung reyoung requested a review from JiayiFeng October 10, 2017 20:54
self._append_initialize_ops_()

def _append_initialize_ops_(self):
attr = copy.deepcopy(self.init_attr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do a deep copy here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

attr and init_attr will share the same object if we do not deepcopy explicitly. I just keep init_attr unchanged.

self.proto = self.block.proto.new_var(name)
try:
self.desc = self.block.desc.var(name)
is_new_var = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

When will the constructor be called for an existing var?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two layers share a same parameter.

attr = copy.deepcopy(self.init_attr)
op_type = attr.pop('type', None)
block = self.block
assert isinstance(block, Block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why shall we do this assert?

Copy link
Collaborator Author

@reyoung reyoung Oct 10, 2017

Choose a reason for hiding this comment

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

Just for IDE type hint.

Copy link
Collaborator

@JiayiFeng JiayiFeng left a comment

Choose a reason for hiding this comment

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

LGTM

@JiayiFeng JiayiFeng merged commit ee22a43 into PaddlePaddle:develop Oct 10, 2017
@reyoung reyoung deleted the feature/parameter branch October 12, 2017 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants