nu-radio / NuRadioMC

A Monte Carlo simulation package for radio neutrino detectors
GNU General Public License v3.0
35 stars 35 forks source link

Python 3.6 compatibility #628

Open sjoerd-bouma opened 8 months ago

sjoerd-bouma commented 8 months ago

Currently, the unit tests fail on python 3.6 (see https://github.com/nu-radio/NuRadioMC/actions/runs/7884786760/job/21514540342). There seem to be two separate issues:

Of course, another option is to deprecate Python 3.6 (as has been done by Python themselves) and simply change the minimum required version.

sjoerd-bouma commented 8 months ago

(FWIW, the tests failing under 3.9 in the linked test run is probably just a random fluctuation, though the error message (4 sigma fluctuation!) is probably inaccurate)

sjoerd-bouma commented 8 months ago

I was able to reproduce the single event test error on python 3.7 by downgrading numpy to 1.19.5 (the last supported version on Python 3.6). Although the random bit generator hasn't changed, it seems Generator.rayleigh has:

import numpy as np
print(f"Numpy version: {np.__version__}")

bit_generator = np.random.Philox(1235)
random_generator = np.random.Generator(bit_generator)
print(random_generator.rayleigh(scale=1, size=5))

gives

Numpy version: 1.19.5
[0.14141684 2.59065742 0.33922295 0.67607868 1.31369548]

but on numpy>1.20:

Numpy version: 1.23.5
[0.19984062 2.15789015 0.4192853  0.5714059  1.71528618]

The results for e.g. random_generator.random still agree, as does the state of the bit_generator; the same thing applies for different (i.e. not Philox) bit generators.

I can't find this change anywhere in the numpy changelog or documentation. Maybe we should raise an issue on the numpy github, I don't think it's desirable/expected behaviour for a random number generator to change between versions.

sjoerd-bouma commented 8 months ago

Linked: https://github.com/numpy/numpy/issues/25824

sjoerd-bouma commented 8 months ago

Update: it turns out it was documented (in three places! Clearly I didn't look well)... changelog numpy philosophy on Generator

In brief - numpy does not guarantee that the Generator.<method> doesn't change, and in fact the implementation of Generator.rayleigh has changed in 1.21. I see four options:

  1. We pin numpy to 1.19. This will break all the tests, and probably causes a bunch of other issues (e.g. at least formally it doesn't support python > 3.8). So this is probably not an option.
  2. We change all random code to use numpy.random.RandomState. This is guaranteed to be stable, and I think can still be used with a more modern BitGenerator (like Philox). As far as I can tell, right now, we would mainly lose a bit of performance on the rayleigh sampling, though that's probably not a huge issue
  3. We deprecate Python 3.6 support
  4. We keep Python 3.6, but accept that simulations are not reproducible unless they are produced with the same version of numpy. This requires some modification of the unit tests to stop Python 3.6 from failing.

Unless we choose (2.) or (4.), we should in any case keep in mind that this issue might happen again if the random methods in numpy are updated again; we should probably pin numpy <= 1.26 (current version) in that case and only upgrade manually, or at least implement a check to make sure the random methods haven't changed with an upgrade.

Thoughts @cg-laser (and anyone else)?

cg-laser commented 8 months ago

Thanks a lot for researching it @sjoerd-bouma. My take on it is that

The random generator are more tricky... It seems that "RandomState" is a legacy interface. Therefore, I prefer the current implementation using the generator class. I don't think it is critical if tests change every few years. We can identify that and update the tests, or force a numpy version and update to a newer version from time to time.

The more important thing is that production runs (i.e., creation of simulation libraries) remain reproducible. If I understand the issue correctly, they will be reproducible as long as the same numpy version is used. And even if not, the likelihood that the random sequences change is probably still low. We only discovered this issue for the first time after several years. We should, however, also dump the numpy version (and python version) to the output files so that the exact environment can be reproduced.

cozzyd commented 8 months ago

Well, EL8 still has python 3.6 by default, which is running in a lot of places (including, e.g. on our server in Greenland). But 1) it's easy enough to install a newer python and 2) it's not clear NuRadioMC needs to run on the server in Greenland...