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

ENH Extend user guide to include classification report #106

Merged

Conversation

BenjaminBossan
Copy link
Collaborator

@BenjaminBossan BenjaminBossan commented Aug 18, 2022

Fixes #103

Description

It is currently already possible to add classification reports to the
model card, but it might not be completely obvious. Therefore, the model
card user guide now includes an example of how to add the classification
report.

Some of the texts in the user guide are rewritten to (hopefully) provide
more clarity and to remove some outdated info.

The model card for plot_model_card.py after the change in this PR can
be seen here.

Implementation

As mentioned, the current code already supports adding the classification
report. However, it is unfortunately necessary to "massage" the report a bit
before it is shown in a useful way. First of all, tabulate cannot parse the
format of the output dict, so we need to go through a DataFrame first. Next, the
"accuracy" key has to be deleted, because it is only a single value, not a
collection, and would otherwise confuse pandas. And finally, the df needs to be
transposed and the index reset. Overall, I'm not sure of this is too hassle for
the user so that we should provide a utility function.

It is currently already possible to add classification reports to the
model card, but it might not be completely obvious. Therefore, the model
card user guide now includes an example of how to add the classification
report.

Some of the texts in the user guide are rewritten to (hopefully) provide
more clarity and to remove some outdated info.
Previous one was very confusing because of the way the data was read
into the DataFrame.
@BenjaminBossan
Copy link
Collaborator Author

@adrinjalali Ready for review.

@adrinjalali
Copy link
Member

This looks pretty good, but I'm wondering if we should put tables which have like more than N (maybe 10) rows in a details tag. The hyperparameter search table is rather long.

@BenjaminBossan
Copy link
Collaborator Author

This looks pretty good, but I'm wondering if we should put tables which have like more than N (maybe 10) rows in a details tag. The hyperparameter search table is rather long.

I had an idea regarding detail tags, maybe that would solve the issue (though in a separate PR): How about we add an additional argument to all add_* methods like detail_tag=False. If True, everything that is added is automatically enclosed in a detail tag. That would give the user more control over that, instead of setting arbitrary limits like 10 rows/columns. WDYT?

@adrinjalali
Copy link
Member

That sounds good to me, and having it in a separate PR sounds good too.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

I'm happy with this. What I have in mind is to have these in our examples, and see their usage, and then figure out a better API to make it easier for users to create a nice modelcard with minimal lines of code kinda thing. Shall I merge?

@BenjaminBossan
Copy link
Collaborator Author

That sounds good to me, and having it in a separate PR sounds good too.
Great, I'll create an issue.

What I have in mind is to have these in our examples, and see their usage, and then figure out a better API to make it easier for users to create a nice modelcard with minimal lines of code kinda thing.

I assume this refers to my comment about how the classification report needs to be massaged to have the right format? If so, I agree.

Shall I merge?
It can be merged from my side.

@adrinjalali adrinjalali merged commit a4b5cb4 into skops-dev:main Aug 18, 2022
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.

Add classification report to model card
2 participants