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

Removed redundant code and modified embedding_modules file #655

Merged
merged 9 commits into from Mar 17, 2020

Conversation

soovam123
Copy link
Contributor

I removed redundant code in embedding_modules file and added them to a new set_embed_para function.

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@w4nderlust w4nderlust 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 this PR. I would change a few things:

  • leave the regularizer part out
  • rename the function to embedding_matrix_on_device
  • move the function to embedding_modules.py

@soovam123
Copy link
Contributor Author

@w4nderlust Could you please clarify your first and last points? The function is still in embedding_modules.py. And for the regularizer part, what are you exactly suggesting?

@w4nderlust
Copy link
Collaborator

@w4nderlust Could you please clarify your first and last points? The function is still in embedding_modules.py. And for the regularizer part, what are you exactly suggesting?

  1. You are right, i thought it was placed in another module, my bad.
  2. The 2 lines if not regularize: regularizer = None should be placed before the call to embed_matrix_on_device not inside. The reason is that I try to avoid if i can functions with side effects like this, so that you then have to return the input.

@soovam123
Copy link
Contributor Author

Aah okay, that is indeed a good precautionary step. Thanks for the insight.

Copy link
Collaborator

@w4nderlust w4nderlust left a comment

Choose a reason for hiding this comment

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

In your last commit you are removing regularize that you introduced previously. That is correct, \regularize\ is never used, but you should pass regularizer as it is provided by the functions calling embedding_matrix_on_device.

To recap embedding_matrix_on_device needs to have a regularizer parameter as it is needed by embedding_matrix and all __call__ functions using embedding_matrix_on_device should doL

if not self.regularize:
             regularizer = None

and then pass the regularizer to the call to embedding_matrix_on_device as they were passing it to embedding_matrix before your edits.

@soovam123
Copy link
Contributor Author

@w4nderlust I am afraid I am not so sure about what you are trying to convey. But, in the code, the function embedding_matrix_on_device already has the regularizer parameter which is passed only after modifying it with a value depending on the regularize parameter in the if statement before the function call.

@w4nderlust
Copy link
Collaborator

Ah you moved it around from the bottom, that's fine then

@w4nderlust w4nderlust merged commit 5ffe45c into ludwig-ai:master Mar 17, 2020
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.

None yet

3 participants