Skip to content
This repository was archived by the owner on Jul 7, 2023. It is now read-only.

Fix bug: "is_generate_per_split" should be set to property. #1322

Merged
merged 1 commit into from
Jan 4, 2019

Conversation

kngxscn
Copy link
Contributor

@kngxscn kngxscn commented Dec 21, 2018

is_generate_per_split in TranslateProblem inherits from Text2TextProblem, so it should be set to property. Otherwise when we train model of TranslateProblem, the judgement in

if self.is_generate_per_split:
will always be true (self.is_generate_per_split returns a method object), even if we overwrite is_generate_per_split (without @property) to return False in the subclass of TranslateProblem.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot googlebot added the cla: no PR author has not signed CLA label Dec 21, 2018
@kngxscn
Copy link
Contributor Author

kngxscn commented Dec 21, 2018

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes PR author has signed CLA and removed cla: no PR author has not signed CLA labels Dec 21, 2018
@kyoheikoyama
Copy link
Contributor

Hi, I faced the same error of ImportError: No module named discrete_domains, when I did PR.

Don't you know how to fix it?

@kngxscn
Copy link
Contributor Author

kngxscn commented Dec 29, 2018

Hi, I faced the same error of ImportError: No module named discrete_domains, when I did PR.

Don't you know how to fix it?

Sorry, I don't know, and I only see this #1319.

Copy link
Contributor

@lukaszkaiser lukaszkaiser left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@lukaszkaiser lukaszkaiser merged commit 948b32b into tensorflow:master Jan 4, 2019
@lukaszkaiser
Copy link
Contributor

Great thanks for correcting this hard-to-see bug that probably affected many people!

@kngxscn
Copy link
Contributor Author

kngxscn commented Jan 5, 2019

It's my pleasure!

kpe pushed a commit to kpe/tensor2tensor that referenced this pull request Mar 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants