search

cmap / cmapPy

Assorted tools for interacting with .gct, .gctx files and other Connectivity Map (Broad Institute) data/tools
https://clue.io/cmapPy/index.html
BSD 3-Clause "New" or "Revised" License
126 stars 76 forks source link

Python3 compatible #54

Closed saksham219 closed 5 years ago

saksham219 commented 5 years ago

Made cmapPy python3 compatible. All test cases are passing in python3 as well and python2.

The changes are

Added tests for python3 in tests_py2 directory in pandasGEXpress (some updates in tests of python2)

saksham219 commented 5 years ago

Hi @oena . Can you review this?

oena commented 5 years ago

Hi @saksham219 -- sorry for the delay, I've been out of town. First, thanks for taking the time to do this! I really appreciate it, and for the most part things look reasonable.

I think the main thing that's missing testing-wise is a test along the lines of test_edge_cases.py but between Python 2 & Python 3 parsers/writers-- while obviously there are some differences in representation between the versions, it's important that the content of GCT and GCTx files doesn't change depending on which version of Python you use, especially because this is now being used for a couple data processing pipelines. For example, we don't want expression values to be slightly different (because of rounding, truncation, etc) depending on if the GCT/X file was written in Python 2 vs. Python 3.

Could you add that please? As long as that's set I'd be happy to accept the PR and re-release the package with your code.

saksham219 commented 5 years ago

@oena . Thank you for replying. I have thought of writing test cases for what you mention. In vague terms, I will make seperate test cases for python2 and python3 for writing/parsing GCT and GCTx files which test parsing of high precision floating values in both. Currently in my PR, the tests for python3 reside in a different directory 'tests_py3' and tests for python2 reside in the original 'tests' directory. I was wondering if this implementation is suitable. If yes, maybe you can integrate the 'tests_py3' directory in the travis-ci testing as well. Let me know if this makes sense.

oena commented 5 years ago

That's a good point, my bad for missing the detail about travis setup earlier. Before proceeding further, let me add the python_3 tests path to the travis yaml file to make sure those tests are ok.

As long as those are set, I think what would work best in terms of overall structure is the following:

In pandasGEXpress/tests/:

Let me know if you have any objections; if not, I'm happy to move around the files and paths for the python 2 files so that it fits into this structure as well.

On your end, I think the only changes this would require would be adding the py2-py3 compatibility tests as we previously discussed, and changing the functional test paths to point to cmapPy/pandasGEXpress/tests/functional_tests/ instead of duplicating them as you currently do.

oena commented 5 years ago

Actually, I looked into this-- not super familiar with using travis on forks-- and it seems (which makes sense) that I can't test your fork for you; you need to modify the .travis.yaml file on your fork to test your new code (details and helpful suggestions here).

Once you have an account, you just need to add the path to your python3_tests folder in cmapPy/.travis.yml and it should be all set to run.

saksham219 commented 5 years ago

Sure. I will work on the two tasks and update this request. I'll also add the python3_tests folder path to travis in my fork.

saksham219 commented 5 years ago

Hi @oena I made the required changes.

oena commented 5 years ago

Fantastic, thanks so much @saksham219!! Looking over the last couple of things now.