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

SaveGraph producing unclear results, when the TChain has no name #10928

Closed
ikabadzhov opened this issue Jul 7, 2022 · 10 comments · Fixed by #14726
Closed

SaveGraph producing unclear results, when the TChain has no name #10928

ikabadzhov opened this issue Jul 7, 2022 · 10 comments · Fixed by #14726
Assignees
Labels
fixathon This issue can be tackled at a ROOT fixathon good first issue improvement in:RDataFrame

Comments

@ikabadzhov
Copy link
Contributor

If the underlying TChain has no name (""), then the output of SaveGraph is far from helpful.
The following script:

#include<ROOT/RDataFrame.hxx>
#include<ROOT/RDFHelpers.hxx>
#include<TSystem.h>

using namespace ROOT;

int main(){
  ROOT::RDataFrame(1).Define("x", "42").Snapshot("t", "f.root");

  TChain ch("");
  ch.Add("f.root?#t");
  RDataFrame df(ch);
  auto df2 = df.Count();
  ROOT::RDF::SaveGraph(df, "res.dot");
  gSystem->Exec("dot -Tpng res.dot -o img_res.png");

  //df.Describe().Print();
}

Produces:
img_res

Example solution: if the chain has no name => call the source node TChain with no name.

(Remark, same comment is applicable for the Describe() action. The commented line would produce Dataframe from TChain in file f.root [...]. Notice the 2 spaces after "TChain".)

CC: @eguiraud @vepadulano

@eguiraud
Copy link
Member

eguiraud commented Jul 7, 2022

:)

@vepadulano
Copy link
Member

Blessed TChains with no name :D I vote for Unnamed: it's one word, can be easily written in the yellow circle above and used in the sentence Dataframe from TChain "Unnamed"

@eguiraud
Copy link
Member

eguiraud commented Jul 7, 2022

Just "Unnamed" is not very descriptive. If the chain has no name SaveGraph can show e.g. "TChain (5 files)" instead.

@ikabadzhov ikabadzhov added this to Issues in Fixed in 6.28/00 via automation Aug 18, 2022
@ikabadzhov ikabadzhov removed this from Issues in Fixed in 6.28/00 Aug 18, 2022
@ikabadzhov ikabadzhov added this to the 6.28/00 milestone Aug 18, 2022
@ikabadzhov
Copy link
Contributor Author

FYI: Added in Milestone 6.28. since this is the default behaviour in the RDatasetSpec when having a chain with different tree names.

I should solve it if noone comes before the release of 6.28.

@guitargeek guitargeek assigned vepadulano and unassigned ikabadzhov Jan 23, 2023
@eguiraud eguiraud removed this from the 6.28/00 milestone Jan 26, 2023
@eguiraud
Copy link
Member

Removed the 6.28 milestone, this is not severe enough to block the release.

@bernhardmgruber
Copy link
Contributor

This might be affected/solved by #13088.

@eguiraud
Copy link
Member

Thanks @bernhardmgruber , unfortunately this is independent of RNTuple. TChains with no name are a thing, in general.

@bernhardmgruber
Copy link
Contributor

Ah, I didn't check the implementation and assumed #13088 was implemented in TChain. I just had a look now. Sorry about the noise then.

@vepadulano vepadulano added the fixathon This issue can be tackled at a ROOT fixathon label Feb 5, 2024
@dpiparo
Copy link
Member

dpiparo commented Feb 15, 2024

Nice change. I proposed a simplification. The tests seem to fail because the change does what it is supposed to do and the reference would need to be adapted.

@martamaja10
Copy link
Contributor

martamaja10 commented Feb 16, 2024

Nice change. I proposed a simplification. The tests seem to fail because the change does what it is supposed to do and the reference would need to be adapted.

Thanks! We're on it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixathon This issue can be tackled at a ROOT fixathon good first issue improvement in:RDataFrame
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants