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

Propagate run Parameters from input to output #1147

Merged
merged 8 commits into from
Jul 24, 2023

Conversation

andresailer
Copy link
Member

@andresailer andresailer commented Jul 18, 2023

BEGINRELEASENOTES

ENDRELEASENOTES

TODO:

  • Check that this works
  • Make it nicer
  • Re-use EventParameters for RunParameter storage? Cannot do that, cannot specialize the ingestParameters function more than once
  • Write release notes

@github-actions
Copy link

github-actions bot commented Jul 18, 2023

Test Results

       6 files         6 suites   7h 36m 29s ⏱️
   358 tests    358 ✔️ 0 💤 0
1 058 runs  1 058 ✔️ 0 💤 0

Results for commit 346b3e0.

♻️ This comment has been updated with latest results.

@andresailer
Copy link
Member Author

As far as I understand, this will only work with HepMC3 >= 3.2.6 https://gitlab.cern.ch/hepmc/HepMC3/-/issues/44

@MarkusFrankATcernch
Copy link
Contributor

@andresailer This issue is highly connectedd to: PR #1140
Are you sure to add parameters as 4 tuples (string, int, float, double) with name-value pairs?
This to me looks quite limiting.
Would it not be better to have predefined xml snippets and then online store maps <string, string> ?
Such snippets could be:

<param name="author-name" type="string"  value="A.Sailer"/>
<param name="generator" type="string"  value="Jetset 8.6"/>
<param name="world-x" type="double" unit="m"  value="30"/>
<param name="world-y" type="double" value="30*m"/>
<param name="center-of-mass-energy" type="energy"  unit="Gev" value="5000.6"/>
<param name="ECAL:encoding" type="encoding"  value="system:8,barrel:3,module:4,layer:6,slice:5,x:32:-16,y:-16"/>
etc.

@andresailer
Copy link
Member Author

Hi Markus, this is related to #1114

I don't understand what you mean though about the 4 tuples.

@MarkusFrankATcernch
Copy link
Contributor

The 4 tuples are:

      std::map<std::string, std::vector<int>>         m_intValues {};
      std::map<std::string, std::vector<float>>       m_fltValues {};
      std::map<std::string, std::vector<std::string>> m_strValues {};
      std::map<std::string, std::vector<double>>      m_dblValues {};

@andresailer andresailer marked this pull request as ready for review July 19, 2023 14:57
@andresailer
Copy link
Member Author

So it probably also works with hepmc before 3.2.6, but the ReaderAscii only parses the run_info when actually reading events, so we skip(-1), which is otherwise a no-op, I hope also for all the other readers (it is for the Root Reader).

@andresailer
Copy link
Member Author

I think the skip(-1) is not working. I am confused.

@MarkusFrankATcernch
Copy link
Contributor

Negative skips can only work for direct access files (root etc.)
For sequential files this generally is difficult unless one wants to keep a seek entry for every event accessed in the file.

@andresailer
Copy link
Member Author

andresailer commented Jul 19, 2023

Negative skips can only work for direct access files (root etc.) For sequential files this generally is difficult unless one wants to keep a seek entry for every event accessed in the file.

I thought the reader would parse all the text until it hits an event but not the event, but I misread the code and the (-1) was exiting even earlier.

https://gitlab.cern.ch/hepmc/HepMC3/-/blob/92f12d3d2af65c2e142c164fe3d7bed77aa766cc/src/ReaderAscii.cc#L54

Now I read an event and then create a new reader with the same file. How bad can this be 🤔

@MarkusFrankATcernch
Copy link
Contributor

No. Look. This cannot be. If we made a design error we should correct it.

@andresailer
Copy link
Member Author

It is a problem with the hepmc reader, not in dd4hep.

@andresailer
Copy link
Member Author

Apparently, segfault is how bad this can be, so back to the drawing board.

@andresailer
Copy link
Member Author

The segfault was due to moving the reader creation to the beginRun only. But the tests were directly calling readparticles, without there being beginRun. Now a reader is created either at begin run, or in readParticles (as before).

Added also a HepMC Version check so that the code should compile...

@andresailer
Copy link
Member Author

As far as I am concerned, this is as good as it gets for the moment.

@andresailer andresailer merged commit 54dcf46 into AIDASoft:master Jul 24, 2023
14 checks passed
@andresailer andresailer deleted the hepMCMeta branch August 19, 2023 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persist HepMC3 GenRunInfo attributes into metadata tree
2 participants