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

Interfacing EvtCalc in FairShip #528

Merged
merged 44 commits into from
Oct 17, 2024
Merged

Conversation

mferril
Copy link
Collaborator

@mferril mferril commented Oct 4, 2024

Vanilla implementation of the EventCalc LLPs decay event sampler in FairShip for inclusive final states.

Additions & modifications to master:

  • FairShip/shipgen/EvtCalcGenerator(.cxx/.h)
  • FairShip/macro/run_simScript.py
  • FairShip/macro/convertEvtCalc.py

To do list (in order of priority):

FYI @maksymovchynnikov

@mferril mferril requested a review from a team as a code owner October 4, 2024 10:26
@olantwin
Copy link
Contributor

olantwin commented Oct 4, 2024

A preliminary remark and a question:

  • The history needs some cleaning up, we can do that once everything else is ready for review. Only the merge commit might be nice to remove already now.
  • Which of the todo items would you consider necessary for the first version and which can we tackle later?

Copy link
Contributor

@olantwin olantwin left a comment

Choose a reason for hiding this comment

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

First pass just reading over the code.

For testing this, could you please provide some example input files and instructions on how to use it?

macro/run_simScript.py Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.cxx Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.cxx Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.cxx Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.cxx Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.h Outdated Show resolved Hide resolved
test.txt Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.h Outdated Show resolved Hide resolved
macro/run_simScript.py Outdated Show resolved Hide resolved
shipgen/EvtCalcGenerator.h Outdated Show resolved Hide resolved
@olantwin
Copy link
Contributor

olantwin commented Oct 7, 2024

At the software meeting it was agreed that the todo items can be tackled later. @mferril, could you please open issues for them?

One thing that would be very nice to have would be some control plots for the expected output for some example channel and input file. Hopefully we can then gradually expand the collection to include more channels.

@olantwin
Copy link
Contributor

@mferril @maksymovchynnikov Any updates?

At the software meeting it was agreed that the todo items can be tackled later. @mferril, could you please open issues for them?

@olantwin
Copy link
Contributor

@yabezsh @anupama-reghunath, did you have a chance to have a look at the pull request?

Overall, the proposed changes are quite small, so we should be able to merge this pretty quickly.

@mferril
Copy link
Collaborator Author

mferril commented Oct 11, 2024

@mferril @maksymovchynnikov Any updates?

At the software meeting it was agreed that the todo items can be tackled later. @mferril, could you please open issues for them?

Hi @olantwin , I have given this lower priority given some conflicting deadlines this week. I am planning to look at it later in the day/weekend. Thanks for understanding!

@olantwin
Copy link
Contributor

Hi @olantwin , I have given this lower priority given some conflicting deadlines this week. I am planning to look at it later in the day/weekend. Thanks for understanding!

Ok, no worries! Good luck with the deadlines!

mferril and others added 2 commits October 11, 2024 16:40
@olantwin
Copy link
Contributor

@yabezsh @anupama-reghunath I'd really appreciate your feedback on your pull request, but will interpret continued silence as approval.

@mferril Could you please provide me a .dat file I can test this with? Apart from that this now looks to be in fairly good shape, and would be ready for automatic fixes, history rewriting etc.

@olantwin
Copy link
Contributor

@mferril This also is still missing an entry in the changelog. I think an paragraph to introduce such a major new feature would be great, maybe with a link to your collaboration meeting presentation.

@mferril
Copy link
Collaborator Author

mferril commented Oct 16, 2024

@yabezsh @anupama-reghunath I'd really appreciate your feedback on your pull request, but will interpret continued silence as approval.

@mferril Could you please provide me a .dat file I can test this with? Apart from that this now looks to be in fairly good shape, and would be ready for automatic fixes, history rewriting etc.

I have just shared with you the input .dat file. To test whether it works, we could think about adding in the future some unit-testing or similar.

Basic commands to launch:
python convertEvtCalc.py -f test_input.dat -o test_folder
then
python run_simScript.py --evtcalc -n 1 -o test_folder -f test_folder/test_input.root

@mferril
Copy link
Collaborator Author

mferril commented Oct 16, 2024

@mferril This also is still missing an entry in the changelog. I think an paragraph to introduce such a major new feature would be great, maybe with a link to your collaboration meeting presentation.

@olantwin this has been changed in the last commit.

@olantwin
Copy link
Contributor

@mferril This also is still missing an entry in the changelog. [..]

@olantwin this has been changed in the last commit.

Thanks! I'll take the liberty to slightly rephrase/reorganise it, but it provides the perfect amount of detail.

@olantwin
Copy link
Contributor

@yabezsh @anupama-reghunath I'd really appreciate your feedback on your pull request, but will interpret continued silence as approval.
@mferril Could you please provide me a .dat file I can test this with? Apart from that this now looks to be in fairly good shape, and would be ready for automatic fixes, history rewriting etc.

I have just shared with you the input .dat file. To test whether it works, we could think about adding in the future some unit-testing or similar.

Basic commands to launch: python convertEvtCalc.py -f test_input.dat -o test_folder then python run_simScript.py --evtcalc -n 1 -o test_folder -f test_folder/test_input.root

Some observations:

  1. When the output dir does not exist, the conversion script crashes. Might be worth creating it.
  2. If the input file does not exist, the EventCalcGenerator crashes instead of exiting cleanly.

@yabezsh
Copy link
Contributor

yabezsh commented Oct 17, 2024

Hi @mferril ,
Thank you for the great work. I tested everything with the files provided and it worked smoothly on lxplus. The code looks good to me too.
I just have one ideological question. The first step is to convert the .dat file from EventCalc to the .root format, which will then be integrated into EventCalc. Will EventCalc be integrated into FairSHiP in the future, or will we use it as an external generator?
And the second question, the new physics particle now has pdg 999, do we plan to have different pdg codes for different models? Or how can we distinguish in the generated files what kind of model has been simulated?
Thanks!

@olantwin
Copy link
Contributor

The first step is to convert the .dat file from EventCalc to the .root format, which will then be integrated into EventCalc. Will EventCalc be integrated into FairSHiP in the future, or will we use it as an external generator?

In my opinion EventCalc should be in a different repository, but ideally our simulation would call it directly without intermediate files. So close integration, and we could even consider maintaining/developing it in the collaboration, but I don't see any advantages and several disadvantages in including it within FairShip.

And the second question, the new physics particle now has pdg 999, do we plan to have different pdg codes for different models? Or how can we distinguish in the generated files what kind of model has been simulated? Thanks!

This is an issue to be fixed after this initial version. For most, no standard PDG codes exist, and using the geantino as we do currently is no long-term solution. In the short-term, we need to differentiate via bookkeeping, but adding new particles to the PDG database similar to what we do for HNL would be better.

@yabezsh I interpret your message as approval?

@mferril
Copy link
Collaborator Author

mferril commented Oct 17, 2024

The first step is to convert the .dat file from EventCalc to the .root format, which will then be integrated into EventCalc. Will EventCalc be integrated into FairSHiP in the future, or will we use it as an external generator?

In my opinion EventCalc should be in a different repository, but ideally our simulation would call it directly without intermediate files. So close integration, and we could even consider maintaining/developing it in the collaboration, but I don't see any advantages and several disadvantages in including it within FairShip.

@yabezsh this is currently under discussion and no final decision has been made yet on the integration within FairShip. We are working to achieve exactly what @olantwin mentioned, which is an internal call to the generator (e.g. as you would call Pythia or any other MC tool). See also Issue #537 .

And the second question, the new physics particle now has pdg 999, do we plan to have different pdg codes for different models? Or how can we distinguish in the generated files what kind of model has been simulated? Thanks!

This is an issue to be fixed after this initial version. For most, no standard PDG codes exist, and using the geantino as we do currently is no long-term solution. In the short-term, we need to differentiate via bookkeeping, but adding new particles to the PDG database similar to what we do for HNL would be better.

Exactly, @olantwin explained it perfectly. Ideally we would issue separate PDG codes in the database for each LLP model we incorporate.

@yabezsh
Copy link
Contributor

yabezsh commented Oct 17, 2024

@mferril @olantwin ok, perfect. That's exactly what I meant, it would be good to be able to call EventCalc directly from FairSHiP so that the user doesn't have to search for .dat files or use EventCalc separately to generate files. Having it as a separate package is then much more practical.
then all is well from my point of view, i approve

@olantwin olantwin removed the request for review from anupama-reghunath October 17, 2024 12:39
@olantwin olantwin merged commit a75b47e into ShipSoft:master Oct 17, 2024
1 check failed
anupama-reghunath pushed a commit to anupama-reghunath/FairShip that referenced this pull request Nov 11, 2024
Introduce a first implementation of the EventCalc decay  event sampler in FairShip for inclusive final states. For further details,  consult the dedicated presentation at the 30th SHiP CM [here](https://indico.cern.ch/event/1448055/contributions/6142341/attachments/2939894/5165450/SHiP_collaboration_meeting_talk_MFerrillo.pdf).
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.

3 participants