Skip to content

MultiLabelEvaluationAsLabelEvaluation #127

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

Closed

Conversation

nezda
Copy link
Contributor

@nezda nezda commented Apr 5, 2021

Description

Ham handed "conversion" of MultiLabel Evaluation (including MultiLabelConfusionMatrix) to corresponding Label Evaluation (including LabelConfusionMatrix) - not meant to actually be merged. How can this be done better?

Motivation

Such a feature would make analysis of MultiLabel models simpler: in particular MultiLabelConfusionMatrix toString() is hard to interpret. Compare

[DenseMatrix(dim1=2,dim2=2,values=
	row 0 [ 2.000000000000000, 0.000000000000000];
	row 1 [ 0.000000000000000, 2.000000000000000];
)
DenseMatrix(dim1=2,dim2=2,values=
	row 0 [ 2.000000000000000, 0.000000000000000];
	row 1 [ 0.000000000000000, 2.000000000000000];
)
DenseMatrix(dim1=2,dim2=2,values=
	row 0 [ 2.000000000000000, 0.000000000000000];
	row 1 [ 0.000000000000000, 2.000000000000000];
)
]

to

          MONKEY  PUZZLE    TREE
MONKEY         2       0       0
PUZZLE         0       2       0
TREE           0       0       2

In many places, constructors, methods, and fields are locked down with final / package private / protected / private which makes tweaking non-essential behaviors challenging. While I understand why from a maintenance and support perspective, it may make sense to allow more access in select places: maybe increased immutability / unmodifiieableX could mitigate risks? Or maybe unstable / unsupported apis could be marked with an annotation (e.g., Beta)? It may be the case that I'm just not groking the public apis provided and those are sufficient to do what I want: to simplify that process for users like me, consider using those apis when implementing things like toString()s so they can be used for reference (this is very often done, but I saw some little opportunities).

@Craigacp
Copy link
Member

Craigacp commented Apr 5, 2021

We originally designed the evaluation packages to be extensible, and to allow users to add their own metrics. Before the initial release we went through and reduced access and locked off that functionality as we didn't have time to document how to do that, and overall the package was too flexible making it hard to ensure sufficient safety. We're willing to relax some of those restrictions, but I think to produce a nicer toString you should already have all the access you need. It is a longer term goal to allow custom metrics and make it simpler to extend the package, but it's not on the immediate roadmap.

As for the actual MultiLabelConfusionMatrix.toString itself, yep that's rough we should rewrite it to print the item labels on the headers at the very least. The MutliLabelConfusionMatrix class is modelled after scikit-learn's multilabel_confusion_matrix, as we figured that would be what people were expecting. I agree that scikit-learn's presentation is a little hard to read, and an alternative form more like LabelConfusionMatrix would be simpler, but I think it also loses information over the current version (or at least it would if the current version had sensible headers, column headings and row headings).

If you want to revise the toString so it's more comprehensible, and also potentially add a form that can print it out in a similar form to LabelConfusionMatrix.toString (though as a separate entry point rather than the default toString), we'd be happy to accept the contribution. You'll need to sign the OCA first (or let us know what email address you've signed it under if you've already done so).

@Craigacp
Copy link
Member

Closed in favour of #128.

@Craigacp Craigacp closed this Apr 13, 2021
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.

2 participants