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

"topk" documentation #147

Open
shiffman opened this issue Jul 11, 2024 · 4 comments
Open

"topk" documentation #147

shiffman opened this issue Jul 11, 2024 · 4 comments

Comments

@shiffman
Copy link
Member

Opening a discussion for topk related to ml5js/ml5-next-gen#166.

  1. The documentation states "The number of top predictions to return, applicable to MobileNet." I think we can remove "applicable to MobileNet" since this is the top predictions that should be returned regardless of classification model.
  2. Should we consider our own name for this property (mapped to topk) and/or a glossary entry?
@alanvww
Copy link
Member

alanvww commented Jul 12, 2024

Hello Dan, in terms of this variable, I wrote it based on our source code

switch (this.modelName) {
        case "mobilenet":
          this.modelToUse = mobilenet;
          const config = handleOptions(
            options,
            {
              version: {
                type: "enum",
                enums: [1, 2],
                default: 2,
              },
              alpha: {
                type: "enum",
                enums: (config) =>
                  config.version === 1
                    ? [0.25, 0.5, 0.75, 1.0]
                    : [0.5, 0.75, 1.0],
                default: 1.0,
              },
              topk: {
                type: "number",
                integer: true,
                default: 3,
              },
            },
            "imageClassifier"
          );
          this.version = config.version;
          this.alpha = config.alpha;
          this.topk = config.topk;
          break;
        case "darknet":
          this.version = "reference"; // this a 28mb model
          this.modelToUse = darknet;
          break;
        case "darknet-tiny":
          this.version = "tiny"; // this a 4mb model
          this.modelToUse = darknet;
          break;
        case "doodlenet":
          this.modelToUse = doodlenet;
          break;
        default:
          this.modelToUse = null;
      }
    }

My understanding is that topk is only related to MobileNet at the moment. And during my meeting with Peter this morning we realized that this variable might not be used even if it is in the code. There is topk in both DoodleNet and DarkNet and the way we have implemented it is this:

classify(img, topk = 10) {
...
}

I think this is why in Jack's problem the kNumber works in imageClassifier.classifyStart(media, ?kNumber, callback);. And I personally prefer the way of using kNumber instead of topk. It provides more space to work with, while the developer can decide the prediction number on the fly instead of setting it at the beginning and tweaking it later.

But either way! I will update the docs to reflect how we decide to implement it :)

@jackbdu
Copy link

jackbdu commented Jul 14, 2024

Just to clarify, passing { topk: 5 } has no impact on MobileNet neither. I agree with @alanvww that it seems reasonable to specify kNumber in .classify()/.classifyStart() and not in the constructor. That also gives us consistency across different models. Also, not sure if it's worth mentioning in the documentation, but I just realized specifying kNumber in .classify()/.classifyStart() has no impact when used with a Teachable Machine model, the model always gives complete results (both of the two classes that I trained it with in my test).

@shiffman
Copy link
Member Author

Ah, I see, I wasn't looking closely at this. I agree, we should focus on the user specifying the number of classes they want to see returned right in the classify() and classifyStart() methods. Happy to call this kNumber of whatever is most clear and friendliest to a beginner. Specifically a topK number when loading the model doesn't have much value for the ml5.js context!

@shiffman
Copy link
Member Author

This has now been implemented in the code (ml5js/ml5-next-gen#179) and we can finalize the documentation, here are some possible next steps:

  1. Remove topk model property? Or should we leave it in the documentation since it does work?
  2. Add an explanation that it can be included as option argument to classify() in the Classify the Image section?
  3. Update classifyStart() and classify() documentation. In the meeting we discussed naming this argument topk but I'm fine with keeping it as kNumber.

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

No branches or pull requests

3 participants