MAINT Small refactor: figure_paths -> extra_sections #89
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
Functionally, nothing changed, this is just a renaming and a little cleanup. The reason is that this attribute can be used for more than just figures, so a more generic name is a better fit. The name "extra_sections" was chosen to mirror the "template_sections". Apart from renaming and adjusting some comments based on that, there was a small change in which the items of the dict are accessed, making it more efficient, and merging to f-strings into one.
|
Ready for review @skops-dev/maintainers |
adrinjalali
approved these changes
Aug 10, 2022
BenjaminBossan
added a commit
to BenjaminBossan/skops
that referenced
this pull request
Aug 10, 2022
Solves skops-dev#87 This feature adds a new method to the Card class called add_table. It adds a generic table to the end of the model card by creating a new section (similar to plots). Any generic table of type dict[str, list[Any]] is supported, as well as DataFrames. However, the main use case is for adding CV results, as e.g. created by GridSearchCV. Implementation I hope you can stomach the type-y implementation Adrin ;) I wanted to make the extra sections more generic in PR skops-dev#89 as a preparation for this PR (thus avoiding a big PR with refactor + new feature). However, the changes in skops-dev#89 were not sufficient (I guess that's the danger of splitting the work). The problem was that the formatting of adding the plots in the save method was very specific to plots, as a markdown link would be added. This, of course, does not work for tables. My solution is that _extra_sections now takes ExtraSection objects as values instead of strings. These objects only need to implement the format method (so technically, str would still be okay). So for a plot, the PlotSection class has a format method that returns a markdown link. And for tables, the TableSection class has a format method that uses tabulate to return a table as string. This approach should hopefully be flexible enough for us to later add new section types if needed. At first glance, the implementation might look overly complex. However, I think it's better than having a bunch of if...else inside of save that formats each possible type differently. But please LMK if you have a better idea, I'm open to suggestions. Examples Here is the hyperparameter table generated by plot_model_card.py: https://huggingface.co/skops/hf_hub_example-b959cadc-ffb3-4f07-856b-2d05d2d8f8e0#hyperparameter-search-results
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.
Functionally, nothing changed, this is just a renaming and a little
cleanup.
The reason is that this attribute can be used for more than just
figures, so a more generic name is a better fit. The name
extra_sectionswas chosen to mirror thetemplate_sections.Apart from renaming and adjusting some comments based on that, there
was a small change in which the items of the dict are accessed, making it
more efficient, and merging to f-strings into one.