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.

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

change: NewVar() to Var() #4725

Merged
merged 15 commits into from
Oct 15, 2017
Merged

change: NewVar() to Var() #4725

merged 15 commits into from
Oct 15, 2017

Conversation

dzhwinter
Copy link
Contributor

@dzhwinter dzhwinter commented Oct 11, 2017

fix #4722
fix #4778

@dzhwinter dzhwinter changed the title Fix/scope change: NewVar() to GetOrCreateVar() Oct 11, 2017
helinwang
helinwang previously approved these changes Oct 11, 2017
Copy link
Contributor

@helinwang helinwang left a comment

Choose a reason for hiding this comment

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

LGTM++

@@ -18,7 +18,7 @@ limitations under the License. */
namespace paddle {
namespace framework {

VarDescBind *BlockDescBind::NewVar(const std::string &name) {
VarDescBind *BlockDescBind::GetOrCreateVar(const std::string &name) {
Copy link

@tonyyang-svail tonyyang-svail Oct 11, 2017

Choose a reason for hiding this comment

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

I think the issue is related to Scope::NewVar. BlockDescBind::NewVar should not be changed.

Copy link
Contributor Author

@dzhwinter dzhwinter Oct 11, 2017

Choose a reason for hiding this comment

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

It's the same with Scope, also do the same logic of find or create. Which we want to make it more clearly.

Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

agree that NewVar is not resonable.
may be the implementation is wrong, an function called NewVar should just create a new var, raise error or return false if an variable exists.
We can add a new interface called GetOrCreateVar, and update the implementation of NewVar.

not just delete it.

FindVar matches NewVar grammerly
but not GetOrCreateVar

@helinwang
Copy link
Contributor

helinwang commented Oct 11, 2017

@Superjom @dzhwinter From my understanding some of the current cpp implementations rely on NewVar returns a var if it's already created. Maybe we can create a new NewVar implementation that return false or raise exception when a var already exists. But change all current NewVar invocation to GetOrCreateVar, because currently noNewVar invocation is handling the error or exception and some of the NewVar invocation actually means to use GetOrCreateVar. And later the original author who wrote the NewVar invocation can change back to NewVar. Otherwise it will take @dzhwinter too much effort to figure out everyone's intension.

@Superjomn
Copy link
Contributor

I think, most of the current usage of NewVar just use it to creates a new Variable semantically.
Better to change the NewVar's implementation to simply create new variable, not just replace all the usages using a more higher level interface called GetorCreateVar.
@helinwang

@dzhwinter
Copy link
Contributor Author

We should only provide one interface to create a variable. Otherwise, the user's code may be ugly mixed with NewVar and GetOrCreateVar.
Throw exception is also not good enough as well. It's not a big deal that people want to reuse a delared variable, soGetOrCreateVar cover the use case of NewVar.

Superjomn
Superjomn previously approved these changes Oct 12, 2017
Copy link
Contributor

@Superjomn Superjomn left a comment

Choose a reason for hiding this comment

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

LGTM

@dzhwinter dzhwinter changed the title change: NewVar() to GetOrCreateVar() change: NewVar() to Var() Oct 12, 2017
@dzhwinter
Copy link
Contributor Author

This PR depends #4728

helinwang
helinwang previously approved these changes Oct 14, 2017
Copy link
Contributor

@helinwang helinwang left a comment

Choose a reason for hiding this comment

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

LGTM!

helinwang
helinwang previously approved these changes Oct 14, 2017
Copy link
Contributor

@helinwang helinwang left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -18,7 +18,7 @@ limitations under the License. */
namespace paddle {
namespace framework {

VarDescBind *BlockDescBind::NewVar(const std::string &name) {
VarDescBind *BlockDescBind::Var(const std::string &name) {
need_update_ = true;
auto it = vars_.find(name);
PADDLE_ENFORCE(it == vars_.end(), "Duplicated variable %s", name);
Copy link
Member

Choose a reason for hiding this comment

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

var should mean getOrCreate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dzhwinter dzhwinter dismissed tonyyang-svail’s stale review October 15, 2017 04:30

So many conflicts, I will merge it ASAP.

@dzhwinter dzhwinter merged commit e593113 into PaddlePaddle:develop Oct 15, 2017
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.

The semantic of Block_desc.Var() is not right Scope::NewVar() not reflecting its implementation.
6 participants