-
Notifications
You must be signed in to change notification settings - Fork 15
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
Resolved #19 added docstring, fixed nproc and removed depth function #29
Conversation
…nformation" This reverts commit ca64a2f.
…emoved depth function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've submitted comments inline to the changes you've made. A few minor updates suggested.
…s and renamed stderr and stdout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sidd,
Looks like you are almost there. A few minor changes and this should be ready for a merge.
…ed additional line in the end, and used o.path.dirname(os.path.abspath(otput.bam))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not catch this before but the ChildProcessError
message in the docstring could explain retcode
rather than refer to the variable. I.e. "return code from subprocessing call"
…r is nproc not int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sidd, we are covering our bases and I think this has been good practice for both of us. A few minor issues with conflicting statements in the docstring vs. behavior of the code.
autometa/common/external/samtools.py
Outdated
retcode = subprocess.call(cmd, stdout=stdout, stderr=stderr, shell=True) | ||
if retcode != 0: | ||
shutil.rmtree(tempdir, ignore_errors=True) | ||
raise ChildProcessError(f'Sort failed, retcode: {retcode}') | ||
else: | ||
shutil.move(temp_outfile, samtools_outfile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will work in according to the behavior we expect for the program, however, if a different exception is raised (for example the user kills the pipeline) will the temporary directory remain? We also need to account for unexpected behavior, which can be difficult due to the nature of the issue, although try/except/finally and specific exception handling can help assess any of the known issues to provide some clarity to the unknown issues.
You might be able to achieve the required behavior (unsure about the exception handling) strictly using the TemporaryDirectory function from the built-in tempfile library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check for FileNotFoundError
(the same way I am checking if nproc
> 0) but I am not exactly sure what all other errors might we encounter and how can we catch-all of them individually.
Won't they be recorded in samtool.err
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sidd, this is great. The use of tempfile.TemporaryDirectory
should significantly simplify this. I've posted a few minor edits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine.
Issues resolved:
depth
function was not being called from any other script. The function is supposed to use samtools to compute the read depth at each position or region, however, it is not being implemented anywhere. It was removed after consulting with @WiscEvan.nproc
was changed tomp.cpu_count()
it now uses all the processors in the system as a default option.run
function more intuitive. Changes here might affect coverage.py file as the function is being called there.