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

DOC User guide for model cards #70

Merged
merged 10 commits into from
Aug 9, 2022
Merged

DOC User guide for model cards #70

merged 10 commits into from
Aug 9, 2022

Conversation

merveenoyan
Copy link
Collaborator

Added user guide for model cards.

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.

This needs some work to expand and explain things a bit better. Also needs to be added to index.rst.

docs/model_card.rst Outdated Show resolved Hide resolved
docs/model_card.rst Outdated Show resolved Hide resolved
leaderboard of the task and the dataset on `Papers with Code
<paperswithcode.com>`__.

Metadata part looks like below:
Copy link
Member

Choose a reason for hiding this comment

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

what are the parts which are important for a scikit-learn model repo?

docs/model_card.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks for writing this. Overall, it already reads quite well. I believe there are some possible improvements though. Apart from what Adrin wrote, I could see e.g. a bit more elaboration at the start about the intent of model cards, so that the reader knows what the goal is (perhaps a link to existing docs would be enough).

docs/model_card.rst Outdated Show resolved Hide resolved
docs/model_card.rst Outdated Show resolved Hide resolved
docs/model_card.rst Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@adrinjalali adrinjalali added this to the 0.1 milestone Aug 3, 2022
Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Content-wise, I'm fine with the addition. However, it doesn't render correctly. Please take a look.

docs/index.rst Show resolved Hide resolved
docs/model_card.rst Outdated Show resolved Hide resolved
docs/model_card.rst Outdated Show resolved Hide resolved
docs/model_card.rst Show resolved Hide resolved
merveenoyan and others added 5 commits August 4, 2022 18:23
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
Copy link
Collaborator

@BenjaminBossan BenjaminBossan 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 also tried it locally and everything renders correctly.

I have made two suggestions but they're not important, up to you to decide if you want to add them.

docs/model_card.rst Outdated Show resolved Hide resolved
docs/index.rst Show resolved Hide resolved
merveenoyan and others added 2 commits August 8, 2022 18:40
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
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.

LGTM. I'll merge and then open a PR with a few suggestions.

@adrinjalali adrinjalali changed the title ENH User guide for model cards DOC User guide for model cards Aug 9, 2022
@adrinjalali adrinjalali merged commit 7b90a74 into skops-dev:main Aug 9, 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.

4 participants