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

ALICE 3: Add xicc injector for pp and PbPb #1688

Merged
merged 10 commits into from
Jul 29, 2024
Merged

Conversation

jesgum
Copy link
Contributor

@jesgum jesgum commented Jul 3, 2024

Adding generators that inject xicc for both pp and PbPb. There is also ini files that make use of these generators so I also added test scripts for respective ini file. However, I'm not sure if the tests are done properly yet so I'm currently marking as a draft for now.

Copy link

github-actions bot commented Jul 3, 2024

REQUEST FOR PRODUCTION RELEASES:
To request your PR to be included in production software, please add the corresponding labels called "async-" to your PR. Add the labels directly (if you have the permissions) or add a comment of the form (note that labels are separated by a ",")

+async-label <label1>, <label2>, !<label3> ...

This will add <label1> and <label2> and removes <label3>.

The following labels are available
async-2023-pbpb-apass3
async-2023-pbpb-apass4
async-2023-pp-apass4
async-2024-pp-apass1
async-2022-pp-apass7
async-2024-pp-cpass0

@jesgum jesgum marked this pull request as ready for review July 3, 2024 14:47
@jesgum
Copy link
Contributor Author

jesgum commented Jul 4, 2024

Hi @sawenzel @benedikt-voelkel ! It seems to be some issues with the tests for the .ini files. At at first the tests for both External passed but failed for Pythia, but now it looks all of them fails, and I am not sure as to why. Could you perhaps have a look and provide some advice? Many thanks!

@sawenzel
Copy link
Contributor

There is an errror saying error: redefinition of 'Pythia8' as different kind of symbol 6-int Pythia8(). I would try to avoid calling your function the same as an existing C++ class.

@jesgum
Copy link
Contributor Author

jesgum commented Jul 10, 2024

Is this concerning the files in the test directory? From the previous ini files that were added, the associated test files also had a Pythia8() function, which as I understand, is fine according to the readme in (https://github.com/AliceO2Group/O2DPG/blob/master/test/README.md) or am I missing something?

@sawenzel
Copy link
Contributor

sawenzel commented Jul 11, 2024

I believe the README is misleading and wrong in this regard. All I can say is that there was a compile error because of duplication of symbols.

I am less familiar with the testing framework but will try to take a look into this. You/ALICE3 seems to be the only place where tests with int Pythia8 are present (although the tests don't actually do anything). Potentially this needs to be changed.

Update: I studied the code a bit. The testing seems to be ok with the int Pythia8 function. No changes necessary. However, your test macros are not really testing anything which defies the purpose. The intention was to verify that the output of the generator is good (for instance non-empty ...). Will need to rethink if we should still force this upon the developers or make it optional.

! Seed is set inside the generator
! If run on the grid, seed is set to job id
! If run locally, seed is set to 0
Random:setSeed = on ! Random seed on
Copy link
Contributor

Choose a reason for hiding this comment

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

We may no longer need the seeding part. All o2::eventgen::GeneratorPythia8 will be seeded automatically from now on.

@nzardosh
Copy link
Contributor

I believe the README is misleading and wrong in this regard. All I can say is that there was a compile error because of duplication of symbols.

I am less familiar with the testing framework but will try to take a look into this. You/ALICE3 seems to be the only place where tests with int Pythia8 are present (although the tests don't actually do anything). Potentially this needs to be changed.

Update: I studied the code a bit. The testing seems to be ok with the int Pythia8 function. No changes necessary. However, your test macros are not really testing anything which defies the purpose. The intention was to verify that the output of the generator is good (for instance non-empty ...). Will need to rethink if we should still force this upon the developers or make it optional.

Hi @sawenzel , we have this problem now with another PR that is stuck (#1703) due to the test failing. Is there a way to resolve this

@ddobrigk
Copy link
Collaborator

I believe the README is misleading and wrong in this regard. All I can say is that there was a compile error because of duplication of symbols.
I am less familiar with the testing framework but will try to take a look into this. You/ALICE3 seems to be the only place where tests with int Pythia8 are present (although the tests don't actually do anything). Potentially this needs to be changed.
Update: I studied the code a bit. The testing seems to be ok with the int Pythia8 function. No changes necessary. However, your test macros are not really testing anything which defies the purpose. The intention was to verify that the output of the generator is good (for instance non-empty ...). Will need to rethink if we should still force this upon the developers or make it optional.

Hi @sawenzel , we have this problem now with another PR that is stuck (#1703) due to the test failing. Is there a way to resolve this

Hi @nzardosh , in the end Jesper and I solved the issue by deliberately typecasting to FairGenerator - this seems to continue working just fine and it solves the CI problem. I'd suggest you do the same :-D

@ddobrigk
Copy link
Collaborator

@benedikt-voelkel @sawenzel can we please merge this? It's been on the system for a few weeks now and we really need this deployed for some ALICE 3 work. Thank you!

@sawenzel sawenzel merged commit 239cd66 into AliceO2Group:master Jul 29, 2024
6 checks passed
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.

4 participants