Conversation
|
@heinc1010 I hope you don't mind me barging in here and proposing an upgrade to Python 3! |
There was a problem hiding this comment.
Pull request overview
This PR upgrades BAMixChecker from Python 2.7 to Python 3, making it compatible with modern Python environments and HPC systems. The upgrade includes comprehensive changes to adapt the codebase to Python 3 syntax and semantics.
- Replaced the deprecated
commandsmodule withsubprocess - Converted all print statements to function calls and exit() to sys.exit()
- Added proper byte-to-string decoding for subprocess outputs
- Updated bitwise operators to logical operators and fixed dictionary iteration
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| README.md | Updated Python version requirement from 2.7 to 3 |
| BAMixChecker.py | Comprehensive Python 3 migration including syntax updates, module replacements, proper string handling for subprocess outputs, operator corrections, and exception handling improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| print " HTML file or Heatmap would not be created properly." | ||
| print " Recommand to install related R packages." | ||
| print " - Required R packages : rmarkdown, ztable, corrplot." | ||
| print("WARNING: Requied R packages are not installed.") |
There was a problem hiding this comment.
Typo: "Requied" should be "Required".
| print("WARNING: Requied R packages are not installed.") | |
| print("WARNING: Required R packages are not installed.") |
| print " - Required R packages : rmarkdown, ztable, corrplot." | ||
| print("WARNING: Requied R packages are not installed.") | ||
| print(" HTML file or Heatmap would not be created properly.") | ||
| print(" Recommand to install related R packages.") |
There was a problem hiding this comment.
Typo: "Recommand" should be "Recommend".
| print(" Recommand to install related R packages.") | |
| print(" Recommend to install related R packages.") |
| print("## ERROR: Reference file is necessary. Use -r option.") | ||
| sys.exit(1) |
There was a problem hiding this comment.
The print exit() statement on line 919 is incorrect syntax. This should be just sys.exit(1) without the print statement wrapping it. The exit() function doesn't return a value to print.
| print dir_path | ||
| print "## ERROR: Option -d and -l are exclusive. Try with one of the options.\n##\t Check 'https://github.com/heinc1010/BAMixChecker' for more information about the options of BAMixChecker." | ||
| exit() | ||
| print(dir_path) |
There was a problem hiding this comment.
The dir_path variable is printed here but it hasn't been assigned a value yet in this code path. At this point, dir_path is still an empty string (initialized on line 875). This print statement should be removed or moved to after line 888 where dir_path is actually assigned, or the variable should reference args.DIR instead.
| print(dir_path) | |
| print(args.DIR) |
| if args.RefVer in [ "hg38","hg19" ]: | ||
| if args.BEDfile != '': | ||
| print "Run for targeted sequecing data" | ||
| print("Run for targeted sequecing data") |
There was a problem hiding this comment.
Typo: "sequecing" should be "sequencing".
| print("Run for targeted sequecing data") | |
| print("Run for targeted sequencing data") |
| print " Refer the instruction or given error message above from GATK." | ||
| exit() | ||
| print("## ERROR: Calling error with GATK HaplotyCaller.") | ||
| print(" Preperation for calling with GATK is instructed in gitHub 'https://github.com/heinc1010/BAMixChecker'.") |
There was a problem hiding this comment.
Typo: "Preperation" should be "Preparation".
| print(" Preperation for calling with GATK is instructed in gitHub 'https://github.com/heinc1010/BAMixChecker'.") | |
| print(" Preparation for calling with GATK is instructed in gitHub 'https://github.com/heinc1010/BAMixChecker'.") |
| print "## The specificity could be lower with the small numbr of SNP loci." | ||
| print "Make an optimized list of SNPs to compare - 'targetSNPs.bed'\n", | ||
| print("## WARNING: The target size is too small, so the number of SNP sites to compare is under 200.") | ||
| print("## The specificity could be lower with the small numbr of SNP loci.") |
There was a problem hiding this comment.
Typo: "numbr" should be "number".
| print("## The specificity could be lower with the small numbr of SNP loci.") | |
| print("## The specificity could be lower with the small number of SNP loci.") |
| if ( dic_sw == None ) & ( dic_un_p == None): | ||
| print "## WARNING : The file names don't have detectable common regulation." | ||
| print " Pairing samples only by genotype." | ||
| if ( dic_sw == None ) and ( dic_un_p == None): |
There was a problem hiding this comment.
Testing for None should use the 'is' operator.
| if ( dic_sw == None ) and ( dic_un_p == None): | |
| if ( dic_sw is None ) and ( dic_un_p is None): |
| if ( dic_sw == None ) & ( dic_un_p == None): | ||
| print "## WARNING : The file names don't have detectable common regulation." | ||
| print " Pairing samples only by genotype." | ||
| if ( dic_sw == None ) and ( dic_un_p == None): |
There was a problem hiding this comment.
Testing for None should use the 'is' operator.
| if ( dic_sw == None ) and ( dic_un_p == None): | |
| if ( dic_sw is None ) and ( dic_un_p is None): |
| if isinstance(stderr, bytes): | ||
| stderr = stderr.decode('utf-8', errors='replace') | ||
| if isinstance(stdoutput, bytes): | ||
| stdoutput = stdoutput.decode('utf-8', errors='replace') |
There was a problem hiding this comment.
Variable stdoutput is not used.
I could not get the original tool to work on the HPC because I was having difficulties getting the older versions of libraries in my conda environment, and I ended up upgrading to the code to use python 3.
I confirmed that the tool works with the provided tutorial data and cleaned up the list of imports.