ENH: Default section for Card.add_* methods #321
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
@skops-dev/maintainers ready for review |
adrinjalali
approved these changes
Mar 21, 2023
adrinjalali
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
The
add_*methods forCardnow have default sections (the one used in the skops template) and no longer set descriptions by default.Noneis 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, manyadd_*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
Cardclass, 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.