Closed BrunoGrandePhD closed 6 years ago
By the way, to remove the print statements in xyalign/xyalign.py
, I had to move the logging.getLogger()
call to the top of the file. I'm not sure if this is correct or if it will work. Please fix as necessary.
Hi Bruno - Thanks!
As I mentioned in our email, unfortunately you had just opened this pull request as I had finished switching all of the docstrings in the package over to Numpy format to be human-readable, yet easily handled by Sphinx. I would prefer to keep them in Numpy format, but open to changing if you feel strongly.
I like the logging switch in xyalign.py. I haven't tried using it yet, but if it works, I think it's great.
Removing unused variable and imports sounds great.
As you were working on this, I independently changed all functions ending in "pass" to return 0 instead. I liked the idea of them returning something. Do you have strong preferences one way or the other?
I have cleaned up the try/except block in variants.hist_read_balance. Does the version in master look good to you?
In xyalign.py, I like the utils.validate_dir function. I also like the new "setup param dictionary" section; did you run any tests to compare its behavior to the previous code?
The code is already pretty polished. All I ended doing are minor edits. Here's a summary of the changes that I've made:
is
instead of colons (:
). To be consistent with the Google Python Style Guide, I've tried to switch all lists to using colons. I've also ensured that the same docstring headings were used throughout the codebase (e.g.Args:
,Returns:
).logging
.# TODO (bgrande)
.Feel free to address my inline comments (tagged with
# TODO (bgrande)
) by pushing to this branch before merging into master. I can help with the merge once you review my pull request.