Some scripts (plot_spherical.py, cpplint.py, ...) are incompatible with Python 3

[beiwang2003]

Bug report

The current plotting script /vis/python/plot_spherical.py only works with Python 2. When using Python 3, it reports the following error message:

[beiwang@tigercpu python]$ python --version
Python 3.6.5 :: Anaconda, Inc.
[beiwang@tigercpu python]$ python plot_spherical.py ../../bin/236bcf9_result/simple_torus.prim.00010.athdf rho rho_10.png --logc --vmin=1.0e-6 --vmax=1.0e0
/usr/licensed/anaconda3/5.2.0/lib/python3.6/site-packages/h5py/__init__.py:36: FutureWarning: Conversion of the second argument of issubdtype from `float` to `np.floating` is deprecated. In future, it will be treated as `np.float64 == np.dtype(float).type`.
  from ._conv import register_converters as _register_converters
Traceback (most recent call last):
  File "plot_spherical.py", line 378, in <module>
    main(**vars(args))
  File "plot_spherical.py", line 170, in main
    vals_left = 0.5 * (data[kwargs['quantity']][(nx3/2)-1, :, :]
IndexError: only integers, slices (`:`), ellipsis (`...`), numpy.newaxis (`None`) and integer or boolean arrays are valid indices

(need to test the other scripts in vis/python/ for compatibility)

Version info

• Compiler and version:

module list
  1) anaconda3/5.2.0                    5) hdf5/intel-16.0/intel-mpi/1.8.16
  2) intel-mkl/2019.0/0/64              6) intel-advisor/2019.0-570901
  3) intel/19.0/64/19.0.0.117           7) intel-vtune/2019.0-570779/64
  4) intel-mpi/intel/2018.3/64

• Hardware and cluster name (if applicable): The login node of Tigercpu

[felker]

I made sure that all .py files in tst/ were compatible with both Python 2 and 3 in #48, but I never checked the remaining Python source code in vis/python/.

Since we document in the Wiki that both are supported, we should either update the documentation or extend these scripts to Python 3 if it is straightforward. Low priority, though.

[felker]

@chaochinyang correctly pointed out to me that the C++ style linting script provided by Google, tst/style/cpplint.py, is also incompatible with Python 3. This can be a problem if the shebang

#!/usr/bin/env python

points to a Python 3 distribution, since the script will check the first file piped to program, '../../src/athena.hpp', and then silently die without checking/reporting any style errors. Although, such a system would technically not be in accordance with PEP 394 which suggests:

If the python command is installed, it should invoke the same version of Python as the python2command (however, note that some distributions have already chosen to have python implement the python3 command)

When invoked, python2 should run some version of the Python 2 interpreter, and python3 should run some version of the Python 3 interpreter.

Nevertheless, the current "silent failure" should be addressed. Some options:

• There are third-party patches (some use the six module) that make it compatible for both Python 2 and 3.
• Could change shebang to #!/usr/bin/env python2, but I assume that many systems/package distributions still don't follow PEP 394 and only install a python binary. (EDIT: I later made this change in af3f7ba on 2018-11-02)
• Hardcode a Python 3 trap in cpplint.py
• Document any restriction to Python 2

Info on the copy of cpplint.py in Athena++:

• Added to repository in commit 4efd31a on 2018-04-18
• Matches google/styleguide version from 2018-04-03:
  google/styleguide@1b206ee
  • Our version still has the "seperated" typo that was fixed on 2018-04-13: google/styleguide@43d512b
• Even though Google's version of their linter has a Python 2 vs. Python 3 switch at the top of the file:

try:
  xrange          # Python 2
except NameError:
  xrange = range  # Python 3

it does not claim compatibility with Python 3 at this time (although there is little-to-no documentation of this, see google/styleguide#277).

• Strangely, the latest cpplint.py lives on the gh-pages branch, not master
• A PR was opened on google/styleguide on 2018-04-15 from a non-Google user that claims to fix the Python 3 incompatibilities (mostly Unicode string decoding issues): Add Python3 Support of cpplint.py and cpplint_unittest.py google/styleguide#349
  • Google seems to ignore such PR
  • Someone working on apache/mesos made a patch based on parts of this PR: apache/mesos@89db66e . Reviewed here: https://reviews.apache.org/r/67055/
• A separate PR was just opened on 2018-10-08 to address Python 3 incompatibilities, but it depends on six module, which is suboptimal: cpplint: Make six-compatible for Python3 google/styleguide#408
  • It mentions the forked version of cpplint.py which is also distributed on PyPI, but the GitHub repo seems to be defunct (no commits for 2 years): https://github.com/cpplint/cpplint/commits/master

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.