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

Image Classifier update #179

Merged
merged 1 commit into from
Aug 22, 2024
Merged

Image Classifier update #179

merged 1 commit into from
Aug 22, 2024

Conversation

OrpheasK
Copy link
Collaborator

This PR fixes the issue #166 that @jackbdu brought into attention and also offers a chance to decide upon for the use/name of the topk parameter in conjunction with kNumber as already mentioned in issue #83 by @lindapaiste.

Also recently mentioned in #83 is the support of local loading of TM models among other subjects, which can be incorporated in this or another PR depending on the priority each aspect will be given.

@OrpheasK OrpheasK requested review from shiffman and ziyuan-linn July 27, 2024 22:37
Copy link
Member

@ziyuan-linn ziyuan-linn left a comment

Choose a reason for hiding this comment

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

This looks good to me! I don't really have a strong opinion on the naming of topk, but perhaps another option could be classes. classes: 4 returns the top 4 classifications.

Personally I think we should only use the ml5.imageClassifier({topk: 4}) and not myImageClassifier.classify(image, 4), so the interface is more consistent with the other models. This might be a breaking change though, and I don't feel strongly about this either.

@lindapaiste
Copy link
Contributor

lindapaiste commented Aug 3, 2024

resultCount might also make sense. Or something along those lines that’s more intuitive and less technical.

@shiffman
Copy link
Member

shiffman commented Aug 6, 2024

Personally I think we should only use the ml5.imageClassifier({topk: 4}) and not myImageClassifier.classify(image, 4), so the interface is more consistent with the other models. This might be a breaking change though, and I don't feel strongly about this either.

@ziyuan-linn can you elaborate on this? What other models follow this pattern? Perhaps it's better to conform the other models to how we are doing it here? I think having the # of classes returned specified with classify() is more intuitive?

@ziyuan-linn
Copy link
Member

@shiffman The other pretrained models like BodySegmentation and FaceMesh only accept a media element and an optional callback on the detection methods and do not accept any kind of options. For example faceMesh.detect(image, gotFaces);. All the options are set during the ml5.faceMesh() call.

I think setting the number of classes returned is unique to ImageClassifier (and maybe soundclassifier?).

I agree that the number of classes on the classify method is more intuitive.

@shiffman
Copy link
Member

Apologies for letting this PR languish for a bit! Based on the discussion we had at our previous Thursday meeting, we decided to allow for passing in the number of desired classes + confidence scores to be returned as an argument to the classify() function itself. We can then extend this pattern to other models that work in a similar way (right now it would only be ml5.neuralNetwork() for the "classification" task. I'll open an issue regarding incorporating this in examples and website documentation.

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.

4 participants