-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Added FaceMatcher.toJSON()/.fromJSON() and LabeledFaceDescriptors.toJSON()/.fromJSON(), with unit tests #397
Conversation
I went back a bit a realized that the toJSON() was not necessary (might as well call JSON.stringify()) as long as fromJSON() did the job right. |
Added FaceMatcher.fromPOJO() Updated FaceMatcher tests and added tests for LabeledFaceDescriptors
Hi thanks for your PR! Some minor remarks:
|
Reverted build/ & dist/ to master
Updated the PR as per your requirements. |
Thanks for your effort and thinking of it I am sorry for my bad example above. Actually your toJSON methods returns a json string, but I think it is better to return a json object here: {
label: ld.label,
descriptors: [
ld.descriptors.map(d => Array.from(d))}
]
} Then you can remove the fromJSON methods and rename fromPOJO to fromJSON, JSON is the same as a "POJO". Right now it also looks like Again, sorry for the inconvenience. |
Gotcha. Coming from C#, Swift, ... toJSON usually means returning a JSON string, but it all makes sense now, thanks to your explanation. As for |
Made .fromJSON() implementations return a POJO Updated the unit tests
Great, now it looks good! @powerdot the change isn't merged yet. If you checked out the branch, the changes have to be built via npm run build. |
Vincent and Jonathan, thank you! Works great. Successful compiled. For community: |
@powerdot Thanks, this works. |
In response to #231.