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

[Hist] Backwards compatibility broken for THnSparseL in 6.32 #15874

Closed
1 task
guitargeek opened this issue Jun 17, 2024 · 4 comments · Fixed by #15880
Closed
1 task

[Hist] Backwards compatibility broken for THnSparseL in 6.32 #15874

guitargeek opened this issue Jun 17, 2024 · 4 comments · Fixed by #15880

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Jun 17, 2024

Check duplicate issues.

  • Checked for duplicates

Description

Backwards compatibility for THnSparseL was broken in 6.32 by #8546, @ferdymercury @vepadulano.

Reproducer

Write a THnSparseL to a file e.g. with ROOT 6.30.06:

void write_thnsparse_l()
{
   std::unique_ptr<TFile> file{TFile::Open("hist.root", "RECREATE")};
   Int_t bins[2] = {10, 20};
   Double_t xmin[2] = {0., -5.};
   Double_t xmax[2] = {10., 5.};
   THnSparseL hist{"hist", "hist", 2, bins, xmin, xmax};
   file->WriteObject(&hist, "hist");
}

Try to read it with ROOT 6.32.00 (or master):

root hist.root -e 'hist'

You'll get this error:

Error in <TClingCallFunc::make_ctor_wrapper>: Failed to compile
  ==== SOURCE BEGIN ====
__attribute__((used)) extern "C" void __ctor_0(void** ret, void* arena, unsigned long nary)
{
   if (!arena) {
      if (!nary) {
         *ret = new THnSparseT<TArrayL>;
      }
      else {
         *ret = new THnSparseT<TArrayL>[nary];
      }
   }
   else {
      if (!nary) {
         *ret = new (arena) THnSparseT<TArrayL>;
      }
      else {
         *ret = new (arena) THnSparseT<TArrayL>[nary];
      }
   }
}

  ==== SOURCE END ====
Error in <TClingCallFunc::ExecDefaultConstructor>: Called with no wrapper, not implemented!
Error in <TClingClassInfo::New()>: Call of default constructor failed to return an object for class: THnSparseT<TArrayL>
Error in <TClass::New>: cannot create object of class THnSparseT<TArrayL>
Error in <TKey::ReadObj>: Cannot create new object of class THnSparseT<TArrayL>
IncrementalExecutor::executeFunction: symbol '_ZN10THnSparseTI7TArrayLE5ClassEv' unresolved while linking [cling interface function]!
You are probably missing the definition of THnSparseT<TArrayL>::Class()
Maybe you need to load the corresponding shared library?
IncrementalExecutor::executeFunction: symbol '_ZN10THnSparseTI7TArrayLE8StreamerER7TBuffer' unresolved while linking [cling interface function]!
You are probably missing the definition of THnSparseT<TArrayL>::Streamer(TBuffer&)
Maybe you need to load the corresponding shared library?

ROOT version

ROOT 6.32.00 upwards.

Additional context

Thanks do @philippe554 for catching this! This issue is important for project HighLO because we're using THnSparseL as a container for reduced data.

@pcanal
Copy link
Member

pcanal commented Jun 17, 2024

Note: THnSparseT<TArrayL> was removed because its underlying type (long)'s size depends on the platforms (32 vs 64 bits).
However, it would have been much to either leave it as is or mark it as deprecated.

We need to fix this and make sure older files can be read.

In the long term, you should consider moving to the newer THnSparseT<TArrayL64> which guarantees the 64 bits on all platforms (we should also make sure that we update our code to that THnSparseT<TArrayL> can be read into a THnSparseT<TArrayL64> (by adding the relevant renaming rule if need be).

@ferdymercury
Copy link
Contributor

ferdymercury commented Jun 17, 2024

Yep, I know. I warned about this problem up to 3 times in that PR, insisting that it had nothing to do with the new feature that I wanted to add:

#8546 (comment)
#8546 (comment)
#8546 (comment)

After 3y, I was forced to give in :s

@pcanal
Copy link
Member

pcanal commented Jun 17, 2024

I also missed it at the time. Sorry.

To avoid this, we should also add test of the backward compatibility (especially the reading of older file).

@vepadulano
Copy link
Member

It's enough to add back the dictionary for THnSparse<TArrayL> to read back the histogram. Now I'm also working on a trasparent conversion from THnSparseL written as THnSparse<TArrayL> to THnSparseL read as THnSparse<TArrayL64>, let's see if I can make that work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants