NuRadioMC icon indicating copy to clipboard operation
NuRadioMC copied to clipboard

Python 3.6 compatibility

Open sjoerd-bouma opened this issue 2 years ago • 6 comments

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:

  • NuRadioReco.detector.RNO_G.rnog_detector uses datetime.datetime.fromisoformat, which was only added in Python 3.7. This should be an easy fix - most straightforward would probably be to switch to astropy.time.Time (@fschlueter?)
  • However, three additional tests fail:
    • Single event test (South Pole)
    • Veff test with noise and phased array
    • Tiny reconstrucution [sic] All of these run successfully but raise an error because their output differs from the reference files. I'm not sure what causes this, maybe whatever random number generator is used changed between Python 3.6 and 3.7?

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 avatar Feb 13 '24 12:02 sjoerd-bouma

(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 avatar Feb 13 '24 13:02 sjoerd-bouma

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 avatar Feb 13 '24 14:02 sjoerd-bouma

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

sjoerd-bouma avatar Feb 14 '24 17:02 sjoerd-bouma

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)?

sjoerd-bouma avatar Feb 15 '24 13:02 sjoerd-bouma

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

  • we should drop python 3.6 support (or is anyone needing 3.6?)

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.

cg-laser avatar Feb 20 '24 00:02 cg-laser

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...

cozzyd avatar Feb 20 '24 03:02 cozzyd

Closed by #832 - Python 3.6 support has been deprecated.

sjoerd-bouma avatar Mar 06 '25 15:03 sjoerd-bouma