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

Python 3.6 compatibility #628

Open
sjoerd-bouma opened this issue Feb 13, 2024 · 6 comments
Open

Python 3.6 compatibility #628

sjoerd-bouma opened this issue Feb 13, 2024 · 6 comments

Comments

@sjoerd-bouma
Copy link
Collaborator

sjoerd-bouma commented Feb 13, 2024

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
Copy link
Collaborator Author

(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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

Linked: numpy/numpy#25824

@sjoerd-bouma
Copy link
Collaborator Author

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
Copy link
Collaborator

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.

@cozzyd
Copy link

cozzyd commented Feb 20, 2024

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

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

No branches or pull requests

3 participants