ENH Allow collapsing any section in card

[lazarust]

Reference Issues/PRs

Fixes #315

What does this implement/fix? Explain your changes.

Implements collapsing all sections and their subsections.

Any other comments?

[BenjaminBossan]

Not sure if it's already meant to be reviewed, but I took a quick look. From my point of view, we don't necessarily need to keep BC by having a folded property and setter. We could also remove the public section.folded and instead have a normal method, like set_folded(bool) which sets section._folded.

I wonder about one scenario:

# pseudo code
card = Card(...)
card.add(foo=...)  # foo is unfolded
card.add(foo/bar=...)  # foo/bar is unfolded
card.add(foo/baz=..., folded=True)  # <= foo/baz is folded
card.select(foo).folded = True  # now foo, foo/bar and foo/baz are folded
card.select(foo).folded = False  # now foo and foo/bar should be unfolded, but what about foo/baz?

IMO, after this chain of actions, foo/baz should still be folded, but this isn't easy to achieve in the current way. I think it would require to adjust the rendering method to "pass down" the folded information from the parent to all subsections, so that whether a section is folded or not depends on whether the folded attribute is true on this section, or any of its parent sections.

[lazarust]

@BenjaminBossan This wasn't really ready for review yet but I appreciate the feedback! I've removed the public folded property and updated the test.

I think your solution would work for figuring out whether a subsection would be folded. To clarify, we wouldn't actually update an individual section's _folded property when the parent's _folded method (when set_folded() is called), right?

[BenjaminBossan]

To clarify, we wouldn't actually update an individual section's _folded property when the parent's _folded method (when set_folded() is called), right?

Yes, I think that's the way to go. So basically, when a parent is set to be being folded, don't change the children, or else the example I gave above would not work correctly.

Implementation-wise, maybe there is a very easy way to achieve this. Looking at this line:

skops/skops/card/_model_card.py

Lines 1321 to 1322 in dd0af39

	if section.subsections:
	yield from self._generate_content(section.subsections, depth=depth + 1)

I think if we change it to:

        if section.subsections and not section.folded:

it should already achieve the goal (if I'm not missing something). So when we recursively visit the sections, skip visiting the children of a section if that section is folded. Then we can keep the .folded attribute as it was. WDYT?

Regarding the test, it should be made more robust by looking at how the card is actually rendered, i.e. using the model_card.format() method. Checking the attribute is an implementation detail and still leaves room for there to be bugs.

[lazarust]

@BenjaminBossan this is ready for you to review again!

[BenjaminBossan]

Thanks for making the changes. Overall, I'm happy that it didn't turn out to be too complicated.

I would like to see the test extended a little bit and I have a design question, please look at the comments.

Apart from those, I think it's time to document the feature since it's now available on all types of sections but it's not easy to discover for users. Would you please make that addition to docs/model_card.rst?

One more design question, do we want to enable folding when calling model_card.add? E.g.:

model_card.add(**{"foo/bar": "Foo/Bar", "foo/baz": "Foo/Baz"}, folded=True)

to add those two sections and fold them? This would be analogous to what we already have for add_table and add_plot. If we want to have this, we could add it in a future PR, though, to keep this one small.

[BenjaminBossan]

Not quite sure why we need a separate _format method here?

It could make sense to have a split like you suggest here if we change PlotSection and TableSection to use _format too. This could be a good idea because we can move the wrap_as_details call to the parent class, avoiding each subclass having to call it. In general, this is a good idea, as it leaves less room for error.

There is a small issue with that suggestion, though. Right now, the PlotSection and TableSection, when folded, don't fold the whole output, but only the plot/figure, whereas the description stays as is. I think there can be an argument for changing that and argument for keeping it, I'm not sure. @adrinjalali what is your opinion?

[adrinjalali]

I think It's okay to have it in one method instead of introducing a private one, and also the whole thing folded instead of just the plot / table. The title should be indicative enough.

[lazarust]

@BenjaminBossan This should be ready for you to look at again!

... This would be analogous to what we already have for add_table and add_plot. If we want to have this, we could add it in a future PR, though, to keep this one small.

Should I create a separate issue for this or put it in the same issue but with a different PR?

[BenjaminBossan]

Thanks, almost perfect, just a minor comment on the documentation.

Should I create a separate issue for this or put it in the same issue but with a different PR?

You could create a separate issue that references the original one. Thanks.

[BenjaminBossan]

I think this last line is not necessary. Instead, it would be better to describe what effect this has on the final model card (not every reader will be familiar with what "folding" means).

[lazarust]

@BenjaminBossan I've updated the documentation. Let me know if you want me to put more of an example of what is returned from model_card.render().

I also created #360 and will try and get that done this week.

[BenjaminBossan]

Thanks so much Thomas, this looks good.