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

Word2vec.nearestFromSet() is always null output #1161

Merged
merged 3 commits into from
Feb 8, 2022

Conversation

Mudasar-Makandar
Copy link
Contributor

Dear ml5 community,

I'm making a Pull Request(PR). Please see the details below.

Fixing a Bug
Word2vec.nearestFromSet() is giving always null output.

Reason:
!miniModel.length is always true because miniModel is object and it's length attribute is undefined.
Hence Word2vec.nearestFromSet() is always giving null output.

Changes:
Object.Keys(miniModel).length will give correct sense while checking for it's length.

Word2vec.nearestFromSet() is giving always null output.

Reason:
!miniModel.length is always true because miniModel is object and length attribute is undefined.
src/Word2vec/index.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@Mudasar-Makandar Mudasar-Makandar 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 review.
I have resolved the typo.

Copy link
Contributor

@joeyklee joeyklee left a comment

Choose a reason for hiding this comment

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

Tagging in @bomanimc our ml5 lead here.
I've reviewed this PR and it looks like it shouldn't introduce any breaking changes (it actually should fulfill the expected behavior!). Since minModel is defined as an object and Object.keys(minModel) will always return an array [], the length property should always exist. And so if there are no elements in the array, then we can successfully null.

Nice one @Mudasar-Makandar !

Copy link
Member

@bomanimc bomanimc left a comment

Choose a reason for hiding this comment

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

LGTM

@joeyklee joeyklee added the SEMVER/patch a version tag for a patch release change label Jan 21, 2022
@joeyklee joeyklee merged commit 82f1638 into ml5js:main Feb 8, 2022
@joeyklee
Copy link
Contributor

joeyklee commented Feb 8, 2022

@all-contributors please add @Mudasar-Makandar for bug code

@allcontributors
Copy link
Contributor

@joeyklee

I've put up a pull request to add @Mudasar-Makandar! 🎉

@joeyklee
Copy link
Contributor

joeyklee commented Feb 8, 2022

NOTE @Mudasar-Makandar -- because word2vec has been disabled in ml5, I wanted to merge this PR in anyways just to recognize your efforts. Even though your changes won't be in effect, I still wanted to make sure that you are shown as a contributor for this bug fix. Thanks so much and apologies that this fix did not make it in before word2vec was disabled in ml5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for release SEMVER/patch a version tag for a patch release change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants