search

impy-project / chromo

Hadronic Interaction Model interface in PYthon
Other
30 stars 7 forks source link

Adding Pythia 8.3.10 to chromo #174

Open gaudu opened 1 year ago

gaudu commented 1 year ago

Attempt to solve https://github.com/impy-project/chromo/issues/171 with @HDembinski ⚡

afedynitch commented 1 year ago

Thanks for starting this! What about this cached directory? How big are the files? Does it cleanup/update itself?

gaudu commented 1 year ago

Thanks for starting this! What about this cached directory? How big are the files? Does it cleanup/update itself?

For each (projectile, target, energy) combination, you will produce three files:

For example, a proton boosted against a target of oxygen-16 at rest would produce files from the following sizes: pO @ 10^3 GeV pO @ 10^7 GeV pO @ 10^11 GeV
.sigfit 45 bytes 46 bytes 42 bytes
.mpi 26 kB 104 kB 122 kB
.sasd.mpi 18 kB 70 kB 80 kB

There is currently nothing in place for the files to cleanup or update themselves, since they are supposed to be generated once by Pythia (according to the user's settings) and kept for future runs using the same (projectile, target, energy) combination. I would need to check if the files are generated in the working directory of the user or in the /examples folder of their Pythia8 installation.

HDembinski commented 1 year ago

Thanks for starting this! What about this cached directory? How big are the files? Does it cleanup/update itself?

We also don't have cleanup routines for the other cached files in place. For example if you update chromo and it needs to download a newer zip file from our server, the old one is not deleted, as far as I am aware.

We should probably wipe the iamdata directory everytime a download of the zip file is triggered, which would do the correct cleanup automatically.

HDembinski commented 1 year ago

@afedynitch @jncots This PR is failing on CI because we are don't have the new zip file for this Pythia8 version uploaded yet. Is it possible to add a new file without making a new release?

Edit: actually it fails even before that because some build code expects that urqmd is in the model list. For development we took all the other models out. @gaudu could you please restore the file default_models.cfg to its original state? And commit?

afedynitch commented 1 year ago

Is it possible to add a new file without making a new release?

Yes, it's possible. @gaudu, please attach the file here if it's not too big

afedynitch commented 1 year ago

a proton boosted against a target of oxygen-16 at rest would produce files from the following sizes

The files are so small that I would rather prefer precomputing all of the common combinations and distribute these as binaries using our zip downloads. Otherwise testing on public workers may take too long. What do you think about this @gaudu & @HDembinski ?

gaudu commented 1 year ago

Is it possible to add a new file without making a new release?

Yes, it's possible. @gaudu, please attach the file here if it's not too big

  1. Here is the zip file, including the pdfdata and xmldoc of Pythia 8.3.10 (size of 13,2 MB which I hope is not too big for this post). Pythia8_v003.zip
  2. One also needs to update the Pythia8_v003 text file which will appear in /chromo/src/chromo/iamdata/Pythia8 with the link to the .zip file before merging again!
gaudu commented 1 year ago

The files are so small that I would rather prefer precomputing all of the common combinations and distribute these as binaries using our zip downloads.

I would not mind generating few files if needed, it really never took that long for me. @afedynitch Could you provide the "common" (projectile, target, center-of-mass energy) combinations you would like to have run?

HDembinski commented 1 year ago

Is it possible to add a new file without making a new release?

Yes, it's possible. @gaudu, please attach the file here if it's not too big

1. Here is the zip file, including the pdfdata and xmldoc of Pythia 8.3.10 (size of 13,2 MB which I hope is not too big for this post).
   [Pythia8_v003.zip](https://github.com/impy-project/chromo/files/12588226/Pythia8_v003.zip)

2. One also needs to update the `Pythia8_v003` text file which will appear in /chromo/src/chromo/iamdata/Pythia8 with the link to the .zip file before merging again!

@afedynitch @jncots You can ignore this, I independently created the zip and uploaded it to the right place.

HDembinski commented 1 year ago

The files are so small that I would rather prefer precomputing all of the common combinations and distribute these as binaries using our zip downloads.

I would not mind generating few files if needed, it really never took that long for me. @afedynitch Could you provide the "common" (projectile, target, center-of-mass energy) combinations you would like to have run?

@afedynitch We can add some precomputed file to the Pythia8 zip file, for example those used in the unit tests, so that the unit tests run faster. However, we cannot cover all use cases of everyone, because we cannot predict how people use Chromo. Therefore, it is anyway needed to generate these cache files on demand. I see no issue with that and it is quite nice to have this functionality in place, as it speeds up Pythia8 a lot when you use it the second time for the same inputs. In practice that happens a lot.

To elaborate why we cannot precompute everything: there is a large but countable set of input particles, but the cache files also depend on ECM, and that's a real number, so it is unbounded.

HDembinski commented 1 year ago

Good news, the tests are all green, except an unrelated Sibyll23StarMixed failure.

FAILED tests/test_final_state.py::test_generator[Sibyll23StarMixed] - AssertionError: Sibyll23StarMixed zero counts for Omega~+, Xi~0, Xi~+, Xi-, Xi0, Omega-2112 27491 False, 130 27728 False, 211 244736 False, 310 27554 False, 321 28965 False, -3334 0 False, -3322 0 False, -3312 0 False, -3222 1795 False, -3122 5876 False, -3112 1762 False, 3112 1725 False, 3122 8684 False, 3222 3300 False, 3312 0 False, 3322 0 False, 3334 0 False, -321 26585 False, -211 233154 False, -2112 16256 False__: [ True  True  True  True  True False False False  True  True  True  True

This test also fails locally. @afedynitch @jncots How could it happen that this test was committed? We should never merge PRs with failing tests.

In my opinion, this PR is a ready to be merged. Having the ability to simulate ion collisions with Pythia8 is a big improvement. Also thanks to the new caching, Pythia8 runs much faster.

There are some issues with the cross section calculation that Chloe made me aware of, which should be tackled in another PR. What to precompute and to put in iamdata is also open, but we can iterate on that independently of this PR.

HDembinski commented 1 year ago

For the record, locally on my computer also another test fails in addition to the Sibyll23StarMixed one:

FAILED tests/test_final_state.py::test_generator[Sibyll23StarMixed] - AssertionError: Sibyll23StarMixed zero counts for Omega~+, Xi~0, Xi~+, Xi-, Xi0, Omega-2112 27491 False, ...
FAILED tests/test_final_state.py::test_generator[UrQMD34] - AssertionError: UrQMD34 zero counts for 2112 1705 False, 130 1812 False, 211 15775 False, 310 1756 False,...
E       AssertionError: UrQMD34 zero counts for 2112 1705 False, 130 1812 False, 211 15775 False, 310 1756 False, 321 1898 False, -3334 1 False, -3322 21 False, -3312 26 False, -3222 73 False, -3122 247 False, -3112 71 False, 3112 96 False, 3122 395 False, 3222 108 False, 3312 19 False, 3322 28 False, 3334 1 True, -321 1735 False, -211 14987 False, -2112 976 False__: [ True  True  True  True  True  True  True  True  True  True  True  True
E           True  True  True  True False  True  True  True]
E       assert False
afedynitch commented 1 year ago

How could it happen that this test was committed? We should never merge PRs with failing tests.

The PR testing this was merged into main before SIBYLL* was merged, or the other way around. We fixed it in #170 . @jncots is about to merge it into main

afedynitch commented 11 months ago

Hi @gaudu,

could you merge from main and see if the tests pass? We merged all of the features we wanted. Then we go through a more thorough review of your PR.

gaudu commented 11 months ago

Hi @gaudu,

could you merge from main and see if the tests pass? We merged all of the features we wanted. Then we go through a more thorough review of your PR.

Hi @afedynitch,

I am still pretty new to git, just to make sure I understood you correctly. Does this mean I should merge my working branch (gaudu/chromo/pythia8310) into my main branch (gaudu/chromo/main) to see if the tests pass, and then you will go through the review of this PR?

afedynitch commented 11 months ago

Ah, I see.

please merge our main into the main of your fork. Then merge your main into your branch. This PR should then update automatically.

Probably, you can merge our main directly into your branch but I remember having some weird problems doing this.

If you succeed, then the conflicts message above should disappear.

afedynitch commented 11 months ago

Oh, once you’ve done it, ‘install -e .’ locally and run ‘pytest -vv’. It should be better to run pytest locally after merging into your main to make sure that the errors you encounter later are not from your configuration.

HDembinski commented 10 months ago

@gaudu @afedynitch I am trying to summarise the todolist for accepting this patch. Is this ok?

gaudu commented 9 months ago

I made few modifications in pythia8.py and test_pythia8.py recently which were unsuccessful at passing the macos and unbuntu tests at the step pythin -m pytest -vv -n 3 which is most likely pointing at failing one of the new test I implemented. I am trying to understand what I did that could have this outcome at the moment starting from: FAILED tests/test_pythia8.py::test_cache_file - AttributeError: 'Pythia8' object has no attribute 'generate' FAILED tests/test_pythia8.py::test_cache_file_heavy_ion - AttributeError: 'Pythia8' object has no attribute 'generate'

On another note, I would like to discuss where exactly we want to have/not have the cache files for (a) the tests, (b) the users? Mostly to know how to answer to @afedynitch comments:

Can the base directory be an optional parameter? If that would be possible then the cache files could be stored as part of the test suite and then nuclei could be tested much faster.

Regarding the caching request further up...I think that this test should be the only one generating and using the cache file. All other tests involving nuclei, should come with caches distributed. These cache files should be in tests/Pythia8_caches/...

HDembinski commented 9 months ago

I have been silent here for a while, but I kinda like the idea of making the cache directory a parameter. Since we anyway want to allow users to turn this feature off and instead of passing a boolean we might as well pass a string (the path) or None to deactivate the caching feature.

On the other hand, I don't see a fundamental difference between these cache files that we generate ourselves and the hardwired ones that we distributed in iamdata. So for testing, I would add the cache files needed for tests to iamdata and make it part of the data that is automatically downloaded from github.

afedynitch commented 9 months ago

I was not aware of the distribution of some default cache files within the binary package... in that case I agree that the natural place for these files to live is within the iamdata directory.

HDembinski commented 9 months ago

@afedynitch

I was not aware of the distribution of some default cache files within the binary package... in that case I agree that the natural place for these files to live is within the iamdata directory.

Sorry, I think I was not clear. Currently, we don't have cache files for the new mpi-stuff in the iamdata directory of Pythia8, but I think that's the place where we should put those cache that we want to use for our unit tests. If we go for that , we have to generate these files and then add them to a new version of the iamdata_v001.zip, and change our code that it finds them.

What we have in there currently is a Pythia8 directory with data files that Pythia8 needs, like the information about particles known to Pythia8, etc. These are not strictly cache files, but data files that Pythia8 needs. I think that the new cache files would nicely fit in there, as they are also "data".

This is analog to QGSJetII, where the corresponding iamdata directory essentially contains a huge binary cache file that QGSJetII can in principle re-create, but we never do that as it takes too long.