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

Show the number of not classified instances for multi-label models (#3964) #4150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

piri-p
Copy link

@piri-p piri-p commented Apr 28, 2024

Fix #3964 - Show the number of not classified instances for multi-label models @suhaibmujahid

Done by

  • Passing extra parameter y_test to calculate the total number of not-classified instances (y_test - classified)
  • Adding table header

Run log attached.

issue-3964-run-log.txt

Copy link
Member

@suhaibmujahid suhaibmujahid left a comment

Choose a reason for hiding this comment

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

Thank you, @piri-p! Please see my comments. Also, please fix the linter (you may want to consider installing pre-commit1).

Footnotes

  1. https://github.com/mozilla/bugbug#auto-formatting

Comment on lines +152 to +153
y_true_count = (test_classes[:, num].tolist()).count(i)
table[i].append(y_true_count - sum(table[i][1:]))
Copy link
Member

Choose a reason for hiding this comment

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

The number of "Not classified" was originally part of the table but dropped earlier:

    # Don't show the Not classified row in the table output
    if "__NOT_CLASSIFIED__" in labels and not is_multilabel:
        confusion_matrix_table.pop(labels.index("__NOT_CLASSIFIED__"))

It may be cleaner to avoid dropping it in the first place.

@@ -499,8 +504,9 @@ def train(self, importance_cutoff=0.15, limit=None):

tracking_metrics["report"] = report

# no confidence threshold - no need to handle 'Not classified' instances
Copy link
Member

Choose a reason for hiding this comment

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

I think this comment is helpful in the context of reviewing this PR. However, without this context, the comment feels out of place (after merging the PR).

@@ -567,8 +573,9 @@ def train(self, importance_cutoff=0.15, limit=None):
labels=confidence_class_names,
)
)
# with confidence threshold - handle 'Not classified' instances by passing y_test
Copy link
Member

Choose a reason for hiding this comment

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

The same case as the comment above.

@piri-p
Copy link
Author

piri-p commented Apr 29, 2024

Hi @suhaibmujahid, thanks you for feedback!

Also, please fix the linter (you may want to consider installing pre-commit

Done :)

The number of "Not classified" was originally part of the table but dropped earlier:

    # Don't show the Not classified row in the table output
    if "__NOT_CLASSIFIED__" in labels and not is_multilabel:
        confusion_matrix_table.pop(labels.index("__NOT_CLASSIFIED__"))

It may be cleaner to avoid dropping it in the first place.

I just run the command in issue-3964-run-log.txt again and checked the confusion matrix of "Not classified" class, it gives [[6, 0], [1, 1]] for Confidence threshold > 0.6.

Now I am not sure how to interpret this. This confusion matrix doesn't seem to correspond to a specific class; but we would like to see the number of 'Not classified' instances in each specific class' visualizer.

In my current implementation, I calculate the number of "Not classified" class-wise, i.e., any non-classified samples from the test set are counted. Here, the test set is of size 200, so any non-classified samples among these 200 are counted. Also, this way, the total sum of all elements in each matrix is precisely the test size, 200.

Says instead of this using current implementation

Current implementation - any non-classified samples from the test set are counted (here: 192, 190)
label: crash - 
     total sum = 1 + 0 + 192 + 0 + 7 + 0 = 200; 
     192 = instances whose ground truth label isn't 'crash', and the model classifies them to neither 'crash' nor 'not crash', i.e. the predicted probability is somewhat ambiguous, e.g. close to 0.5.
      0 = instances whose ground truth label is 'crash', and the model classifies them to neither 'crash' nor 'not crash'
╒════════════╤═════════════════╤═════════════════╤══════════════════╕
│            │   0 (Predicted) │   1 (Predicted) │   Not classified │
╞════════════╪═════════════════╪═════════════════╪══════════════════╡
│ 0 (Actual) │               1 │               0 │              192 │
├────────────┼─────────────────┼─────────────────┼──────────────────┤
│ 1 (Actual) │               0 │               7 │                0 │
╘════════════╧═════════════════╧═════════════════╧══════════════════╛

label: memory - total sum = 7 + 0 + 190 + 0 + 1 + 3 = 200
╒════════════╤═════════════════╤═════════════════╤══════════════════╕
│            │   0 (Predicted) │   1 (Predicted) │   Not classified │
╞════════════╪═════════════════╪═════════════════╪══════════════════╡
│ 0 (Actual) │               7 │               0 │              190 │
├────────────┼─────────────────┼─────────────────┼──────────────────┤
│ 1 (Actual) │               0 │               1 │                2 │
╘════════════╧═════════════════╧═════════════════╧══════════════════╛

We would have this using confusion matrix

Implementation using   __NOT_CLASSIFIED__ in confusion_matrix_table (here: A, B)
label: crash
╒════════════╤═════════════════╤═════════════════╤══════════════════╕
│            │   0 (Predicted) │   1 (Predicted) │   Not classified │
╞════════════╪═════════════════╪═════════════════╪══════════════════╡
│ 0 (Actual) │               1 │               0 │              A   │
├────────────┼─────────────────┼─────────────────┼──────────────────┤
│ 1 (Actual) │               0 │               7 │                B │
╘════════════╧═════════════════╧═════════════════╧══════════════════╛

label: memory
╒════════════╤═════════════════╤═════════════════╤══════════════════╕
│            │   0 (Predicted) │   1 (Predicted) │   Not classified │
╞════════════╪═════════════════╪═════════════════╪══════════════════╡
│ 0 (Actual) │               7 │               0 │              A   │
├────────────┼─────────────────┼─────────────────┼──────────────────┤
│ 1 (Actual) │               0 │               1 │                B │
╘════════════╧═════════════════╧═════════════════╧══════════════════╛

I think this comment is helpful in the context of reviewing this PR. However, without this context, the comment feels out of place (after merging the PR).

Should I remove these comments or how would you prefer it?

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.

Show the number of not classified instances for multi-label models
2 participants