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 2/3 compatibility #18

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

nvictus
Copy link

@nvictus nvictus commented Aug 14, 2017

This is probably still incomplete and requires more testing, so maybe keep in another branch until ready. In short, the consequential changes:

  • shebang and executable permissions added to HiCPlotter.py. Should have no effect on windows.
  • print statements replaced with print functions and real division activated using __future__ imports
  • xrange replaced with range
  • Deprecated for line in file.xreadlines() replaced with for line in file:
  • max in cmatrix = log2(pow(2, ceil(log2(max(matrix))/log2(2)))) replaced with np.nanmax

Issues that will probably arise will involve integer division and incompatibility of comparing objects of different types (e.g. int > list). Other hangups can most likely be resolved with the six package.

Style-wise, the changes are:

  • tabs to spaces :)
  • other pep8 formatting changes in the upper functions, but not comprehensive
  • added some numpydoc-style docstrings
  • deleted lots of .DS_Store junk files and added a .gitignore

EDIT: oh, and also, fixed the default end point for read_Cooldata to end = c.chromsizes[chrom], as we discussed :P

@lachlansimpson
Copy link

These changes look good on the face of it. I've only just found this now, having already made HiCPlotter exec and added the #!/bin/env python.

Any reason for not merging?

@akdemirlab
Copy link
Owner

Million thanks to Nezar! 😃
No reason for not merging, I just need some free time for testing python3 functions.

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

Successfully merging this pull request may close these issues.

3 participants