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

[RDF] add HTML display #17106

Merged
merged 6 commits into from
Dec 11, 2024
Merged

Conversation

silverweed
Copy link
Contributor

This Pull request:

builds on #17081 and adds the option to print a RDisplay as a HTML table.
Additionally, it uses the HTML display by default as a repr for notebooks.

This is a sample output:
image

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Thank you, this is great! I have a couple of minor comments to improve the already very nice changes. As a side thought, these changes also change the behaviour of df.Display in the case of interactive use, from lazy to instant, i.e. now the RDataFrame will run the Display action immediately to print the output table to screen. I believe that's more than valid for interactive use, but we should probably document this. Can you add a line to the release notes (for 6.36 at this point)? Then we should also add some doc about this in the docstrings that can be found in _rdataframe.py

Comment on lines +19 to +24
def repr(klass):
import ROOT
opts = ROOT.RDF.RDisplay.RPrintOptions()
opts.fFormat = ROOT.RDF.RDisplay.EPrintFormat.kHtml
return klass.AsString(opts)
klass._repr_html_ = repr
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add for the non-IPython interactive use case

klass.__repr__ = lambda self: self.AsString()

which will show the markdown table which is friendlier for terminal-based interaction

return {};
}

std::string RDisplay::AsStringHtml() const
Copy link
Member

Choose a reason for hiding this comment

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

Can you also add vertical separators between different columns of the table?

Copy link

github-actions bot commented Nov 27, 2024

Test Results

    18 files      18 suites   4d 7h 22m 57s ⏱️
 2 664 tests  2 662 ✅ 0 💤  2 ❌
46 234 runs  46 212 ✅ 0 💤 22 ❌

For more details on these failures, see this check.

Results for commit 5605fae.

♻️ This comment has been updated with latest results.

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

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

Fantastic!!!

@vepadulano
Copy link
Member

I suggest squashing the commits and applying clang-format suggestions if it makes sense before merging

@silverweed silverweed merged commit 8ca70ee into root-project:master Dec 11, 2024
9 of 21 checks passed
@silverweed silverweed deleted the rdf_display_html branch December 11, 2024 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants