Skip to content

Rdfd print adjusted#9105

Merged
eguiraud merged 2 commits into
root-project:masterfrom
ikabadzhov:RDFD_Print_Adjusted
Oct 13, 2021
Merged

Rdfd print adjusted#9105
eguiraud merged 2 commits into
root-project:masterfrom
ikabadzhov:RDFD_Print_Adjusted

Conversation

@ikabadzhov

@ikabadzhov ikabadzhov commented Oct 12, 2021

Copy link
Copy Markdown
Contributor

This Pull request: [Fix #6981] RDisplay::Print to list custom number of elements from collections

Changes or fixes:

AddCollectionToRow() modified so that the first 10 elements of long collections
are to be printed, then followed by "...". Previously, only the first and last
elements of collections of size >=3 were printed.

Print() logic is now simplified, since after "..." there is no guarantee
that there is going to be a next element.

Additional paramter to the constructor of the the RDisplay() was added,
that is the nMaxCollectionElements. It determines, number of elements to be
shown from long collction. Default is 10.

Corresponding tests were adapted and additional ones were added to check
the new functionality.

Regarding #6981 - to display the whole collection, please use AsString().

Checklist:

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

This PR fixes #6981

@ikabadzhov
ikabadzhov requested a review from eguiraud as a code owner October 12, 2021 14:36
@phsft-bot

phsft-bot commented Oct 12, 2021

Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot

phsft-bot commented Oct 12, 2021

Copy link
Copy Markdown

Build failed on mac11.0/cxx17.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@eguiraud

eguiraud commented Oct 13, 2021

Copy link
Copy Markdown
Contributor

@ikabadzhov please squash the two commits into one and do a force-push of this branch, it will re-trigger the build and we can merge when it's done.

@eguiraud eguiraud mentioned this pull request Oct 13, 2021
Closed
2 tasks
eguiraud
eguiraud suggested changes Oct 13, 2021
View reviewed changes

@eguiraud eguiraud left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Only minor things, congrats on fixing an issue and simplifying the code at the same time

Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/inc/ROOT/RDF/RInterface.hxx Outdated
Comment thread tree/dataframe/src/RDFDisplay.cxx Outdated
@ikabadzhov
[Fix root-project#6981] RDisplay::Print to list custom number of elem…
f1c4392 
…ents from collections

AddCollectionToRow() modified so that the first 10 elements of long collections
are to be printed, then followed by "...". Previously, only the first and last
elements of collections of size >=3 were printed.

Print() logic is now simplified, since after "..." there is no guarantee
that there is going to be a next element.

Additional paramter to the constructor of the the RDisplay() was added,
that is the nMaxCollectionElements. It determines, number of elements to be
shown from long collction. Default is 10.

Corresponding tests were adapted and additional ones were added to check
the new functionality.

Regarding root-project#6981 - to display the whole collection, please use AsString().
@phsft-bot

phsft-bot commented Oct 13, 2021

Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@ikabadzhov @eguiraud
Apply suggestions from code review
1a8a4ef 
Co-authored-by: Enrico Guiraud <enrico.guiraud@cern.ch>
@phsft-bot

phsft-bot commented Oct 13, 2021

Copy link
Copy Markdown

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, ROOT-ubuntu2004/soversion, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot

phsft-bot commented Oct 13, 2021

Copy link
Copy Markdown

Build failed on mac11.0/cxx17.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot

phsft-bot commented Oct 13, 2021

Copy link
Copy Markdown

Build failed on windows10/cxx14.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Failing tests:

@eguiraud
eguiraud merged commit 4de17ec into root-project:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@eguiraud eguiraud eguiraud approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@ikabadzhov ikabadzhov

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[DF] Display should list all elements of a collection by default, like TTree::Scan

3 participants

@ikabadzhov @phsft-bot @eguiraud