litebird / litebird_sim

Simulation tools for LiteBIRD
GNU General Public License v3.0
18 stars 13 forks source link

Change python version in documentation + issue with python 3.9.17 #263

Closed marcobortolami closed 1 year ago

marcobortolami commented 1 year ago

Hi:) I create one Issue for, actually, two problems:

  1. With #254 we dropped support for Python 3.7 and 3.8. However, in installation.rst (and in the slides that @nraffuzz and I prepared as tutorial) we say to the users to run conda create -n lbs_env python=3.8. These two files should be modified. I can take care of it. Which python version should we suggest?
  2. I made a freshly new environment for lbs because I was using Python 3.8 before. I didn't know which Python 3.9.x to choose, so I chose the latest one, i.e. 3.9.17. However, the tests are not working (below a summary of what I did). Errors involve detectors.py. Should I use an older 3.9.x version? By solving point 1. above, also this could be solved.
conda create --name lbs_env python=3.9.17
[...]

conda activate lbs_env

git branch
* master
[...]

git pull
Already up-to-date.

pip install .
[...]

./bin/run_tests.sh
Skipping .ipynb files as Jupyter dependencies are not installed.
You can fix this by running ``pip install "black[jupyter]"``
All done! ✨ 🍰 ✨
58 files would be left unchanged.
./litebird_sim/detectors.py:134:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
./litebird_sim/detectors.py:171:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`
./litebird_sim/detectors.py:275:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()`

All the 3 errors refer to instructions where we do type(whatever) != / == whatever_type

ziotom78 commented 1 year ago

Argh, yes, that error is caused by the fact that I have updated the dependency on flake8. (The older versions didn't complain about this.)

master has the newest flake8 as its dependency, but the fix involving isinstance() is still in PR #260. If we manage to review #260 in time, we can wait until we merge it, otherwise we should backport that fix. (It's just a matter of copying-and-pasting three lines of code…)

ziotom78 commented 1 year ago

Regarding the first point, we can just ask for Python 3.9 and let Conda pick the latest bugfix release.

marcobortolami commented 1 year ago

If I do conda create --name lbs_env python=3.9, then version 3.9.16 is used. The errors in point 2. above remain, as expected. Since it's just a minor change in the doc (8 --> 9), we could update installation.rst in #260 and solve all these problems with that PR:) I'll commit this now and review that PR tomorrow:) I've also updated the slides.

ziotom78 commented 1 year ago

With #260 being merged, perhaps this issue can be closed? installation.rst has been updated to 3.9, and I believe that the problem with flake8 is solved, right?

marcobortolami commented 1 year ago

Regarding the slide and documentation update: ok. Regarding the flake8 problem: ok.

However, after pulling the latest commits of master, I have problems with TOAST. Since it could be related, let's discuss it here and then close this issue. Below a list of self-explanatory commands that lead me to the error. I removed unnecessary outputs.

conda create --name lbs_env python=3.9
conda activate lbs_env
git branch
    * master
git pull
    Already up-to-date.
pip install .
./bin/run_tests.sh
    [everything ok as long as toast is not involved]
pip install toast
./bin/run_tests.sh
    ========================================================= short test summary info ==========================================================
    FAILED test/test_toast_destriper.py::test_basic_functionality - AssertionError: 
    FAILED test/test_toast_destriper.py::test_destriper_ecliptic - AssertionError: 
    FAILED test/test_toast_destriper.py::test_destriper_galactic - AssertionError: 
    ========================================== 3 failed, 94 passed, 28 warnings in 110.63s (0:01:50) ===========================================

Some more details about the failing tests: 1) test_basic_functionality: it should be just matter of reshaping

>       np.testing.assert_allclose(actual=mismatch, desired=mismatch[0])
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0
E           
E           (shapes (1, 9), (9,) mismatch)
E            x: array([[2., 2., 2., 2., 2., 2., 2., 2., 2.]])
E            y: array([2., 2., 2., 2., 2., 2., 2., 2., 2.])

2) test_destriper_ecliptic: I don't know what's happening here, maybe it's related to a type change (I had the warnings reported below) from float32 to float64, for which the precision modification gives the mismatch

>       run_destriper_tests(tmp_path=tmp_path, coordinates=CoordinateSystem.Ecliptic)
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0
E           
E           Mismatched elements: 20 / 3072 (0.651%)
E           Max absolute difference: 1.38266449e-08
E           Max relative difference: 4.08485077e-07
E            x: array([0.355732, 0.311   , 0.354714, ..., 0.      , 0.      , 0.      ])
E            y: array([0.355732, 0.311   , 0.354714, ..., 0.      , 0.      , 0.      ],
E                 dtype=float32)
WARNING  root:__init__.py:55 converting pointings for 0A from float32 to float64
WARNING  root:__init__.py:55 converting pointings for 0B from float32 to float64

3) test_destriper_galactic: same as 2)

>       run_destriper_tests(tmp_path=tmp_path, coordinates=CoordinateSystem.Galactic)
E           AssertionError: 
E           Not equal to tolerance rtol=1e-07, atol=0
E           
E           Mismatched elements: 79 / 3072 (2.57%)
E           Max absolute difference: 1.4553065e-08
E           Max relative difference: 8.06728232e-07
E            x: array([0., 0., 0., ..., 0., 0., 0.])
E            y: array([0., 0., 0., ..., 0., 0., 0.], dtype=float32)
WARNING  root:__init__.py:55 converting pointings for 0A from float32 to float64
WARNING  root:__init__.py:55 converting pointings for 0B from float32 to float64
marcobortolami commented 1 year ago

Solved by #260 and #265.