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: Default section for Card.add_* methods #321

Merged

Conversation

BenjaminBossan
Copy link
Collaborator

Description

The add_* methods for Card now have default sections (the one used in the skops template) and no longer set descriptions by default. None is no longer a valid argument for section.

This is useful because it allows us to completely remove all the template checks in the diverse add_* methods. Before this change, many add_* methods had to check the template and if it was a custom template, would raise an error when the section is not indicated.

All methods have been homogenized in that they no longer add a description by default and put a placeholder there if empty. This means that after this PR is merged, the same code will sometimes produce a different looking model card.

Comment

For the Card class, this is a nice simplification of the code. The main change affects the unit tests. All tests that used to raise when using a custom template without a section argument no longer raise but use the default template. Furthermore, since some default descriptions have been removed, other tests had to be touched too. Apart from that, some minor cleanups in the tests.

Description

The add_* methods for Card now have default sections (the one used in
the skops template) and no longer set descriptions by default. None is
no longer a valid argument for section.

This is useful because it allows us to completely remove all the
template checks in the diverse add_* methods. Before this change, every
add_* method had to check the template and if it was a custom template,
would raise an error when the section is not indicated.

All methods have been homogenized in that they no longer add a
description by default and put a placeholder there if empty. This means
that after this PR is merged, the same code will sometimes produce a
different looking model card.

Comment

For the Card class, this is a nice simplification of the code. The main
change affects the unit tests. All tests that used to raise when using a
custom template without a section argument no longer raise but use the
default template. Furthermore, since some default descriptions have been
removed, other tests had to be touched too. Apart from that, some minor
cleanups in the tests.
@BenjaminBossan
Copy link
Collaborator Author

@skops-dev/maintainers ready for review

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.

Love this, merging the latest main to double check before merging.

@adrinjalali adrinjalali changed the title ENH: Default section for add_* methods ENH: Default section for Card.add_* methods Mar 21, 2023
@adrinjalali adrinjalali merged commit 7ae70ed into skops-dev:main Mar 21, 2023
@BenjaminBossan BenjaminBossan deleted the ENH-add-methods-default-sections branch March 21, 2023 17:24
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