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

Clarifying comments and minor code clean-up in Sketch-RNN #724

Merged
merged 1 commit into from
Jun 20, 2017

Conversation

colinmorris
Copy link
Contributor

  • explanation of stroke-3 and stroke-5 formats

  • clarification of some internal model details

  • simplify/remove some circuitous/redundant code

Mostly cleaned up versions of comments I left for myself as I was trying to understand the source. (e.g. it's really hard to construct a google search for a definition of MDN that isn't "Mozilla developer network"). Hoping they might be useful for others.

There should be no functional changes.

* explanation of stroke-3 and stroke-5 formats

* clarification of some internal model details

* simplify/remove some circuitous/redundant code
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@colinmorris
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@@ -244,7 +244,9 @@ def build_model(self, hps):
self.num_mixture = hps.num_mixture

# TODO(deck): Better understand this comment.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure who deck is, but I hope he would find the new version of this comment more understandable. :)

self.random_scale_factor = random_scale_factor # data augmentation method
self.limit = limit # removes large gaps in the data
# Removes large gaps in the data. x and y offsets are clamped to have
# absolute value no greater than this limit.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't totally understand this part. I would think "large gaps" means large distances between a "pen up" and "pen down" state, but from my reading of preprocess(), it seems like this actually clamps all offsets. Meaning it would also shrink line segments having an x or y component greater than limit. Is that intentional? Or maybe in practice this doesn't happen because some earlier preprocessing means there are no line segments with offsets greater than 1000 or other typical limit values?

@jesseengel jesseengel requested a review from hardmaru June 10, 2017 17:37
@colinmorris
Copy link
Contributor Author

gentle ping

Copy link
Contributor

@hardmaru hardmaru left a comment

Choose a reason for hiding this comment

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

Thanks for the improvements! This went under the radar-

@hardmaru hardmaru merged commit a65505c into magenta:master Jun 20, 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.

3 participants