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

Rename 'type' parameter to fix masking built-in function name #63

Merged
merged 3 commits into from Apr 5, 2019

Conversation

Projects
None yet
3 participants
@iamaziz
Copy link
Contributor

commented Apr 5, 2019

Related Issue(s)

  • Fix issue #44

Description of Changes

  • Rename the parameter type in models.get_embeddings() to embedding_type

@rorymcgrath rorymcgrath self-assigned this Apr 5, 2019

@iamaziz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

My bad! I just noticed the documentation needs to be updated for this as well http://docs.ampligraph.org/en/latest/examples.html#get-the-embeddings I just updated it now. But not sure how to rebuild the docs.

@rorymcgrath

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Hi iamaziz, thank you for the commit.
The get_embeddings() method will be used regularly and calling the parameter type_ may be a bit confusing.
Instead embedding_type might better explain the parameter.

@rorymcgrath

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Sphinx is used to generate the documentation. If you want to update your local documentation you can navigate to the doc directory and type

make html

This will regenerate the documentation and place it in the

docs/_build/html

directory.
Once the pull request is merged the official documentation will be updated automatically by the Jenkins server.

@iamaziz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Hey, thanks for doc instructions. Yea perhaps embedding_type makes more sense :), should I go ahead and update the PR to embedding_type?

@rorymcgrath

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

That would be great :)

@iamaziz

This comment has been minimized.

Copy link
Contributor Author

commented Apr 5, 2019

Sure thing, here you go.

@rorymcgrath rorymcgrath merged commit ac515bc into Accenture:master Apr 5, 2019

@rorymcgrath

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Looks good.

@sumitpai sumitpai added this to the 1.0.2 milestone Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.