KN Picard Metrics Aggregation Script #364
Conversation
There was a problem hiding this comment.
@krithikanathamuni Thank you for putting this together. I made a few general comments on code structure and formatting, as well as more specific suggestions on code refactoring. Let me know if you have any questions!
| @@ -0,0 +1,276 @@ | |||
| def read_in(file_name, label): | |||
There was a problem hiding this comment.
You can be more descriptive with the function name here. You could also consider adding a docstring with a description of the function and potentially inputs/outputs, where you can describe the format of the file_name file, and what does the label represent.
Versions of python > 3.5 also support typing (https://docs.python.org/3/library/typing.html) - you can specify types of the inputs, and function output. This really helps with readability and especially if you are reading someone else's code.
How detailed you want to be depends on whether this script will be used or extended again by you or other people in the future.
| intable = line.startswith(label) | ||
| if intable == True: |
There was a problem hiding this comment.
You can replace these two lines with
| intable = line.startswith(label) | |
| if intable == True: | |
| if line.startswith(label): |
| newlist = [] | ||
| wgsfile = file_name | ||
|
|
||
| with tfio.gfile.GFile(file_name, "r") as inp: |
There was a problem hiding this comment.
Is there a reason for using TensorFlow's GFile here? Can this be replace with Python's built-in open method?
| def read_in(file_name, label): | ||
| started = False | ||
| newlist = [] | ||
| wgsfile = file_name |
| newlist.append(line.rstrip('\n')) | ||
| if started and line == '\n': | ||
| return newlist | ||
| break |
There was a problem hiding this comment.
This break statement is not reachable.
|
|
||
| def sequencing_artifact_metrics(table): | ||
| sadfone = pd.DataFrame(table.loc[1]).T.reset_index(drop = True) | ||
| sadf1 = sadfone.add_suffix("_1").drop(['SAMPLE_ALIAS_1', 'LIBRARY_1', 'WORST_CXT_1', |
There was a problem hiding this comment.
This can also be refactored to get rid of the duplicate code, but also to store the strings SAMPLE_ALIAS, SAMPLE_ALIAS_1 strings into variables. This way if the name of the Picard attribute changes you only need to change one variable - but also there is less chance of having a typo somewhere that will break the code.
| return SAMresult | ||
|
|
||
| def windows(table, window): | ||
| if len(table.columns) == 2: |
There was a problem hiding this comment.
I think this is fine to use in the script, but in general having the output of the function be conditioned on the hardcoded variable like this is a sign that the code should be refactored.
One way to refactor it is to rewrite this code in an object-oriented manner. Again, I don't think it is necessary here but for code that is more than few hundred lines long it's a good idea, and it will provide more readability and robustness.
| table_name = "sample" | ||
| samples = pd.read_csv(io.StringIO(fiss.fapi.get_entities_tsv(project, workspace, 'sample').text), sep='\t') | ||
| samples.rename(columns = {'entity:sample_id':'sample'}, inplace = True) | ||
| # specificcolumns = samples[['alignment_summary_metrics', 'base_distribution_by_cycle_table', 'gc_bias_summary_metrics', |
| # dropemptycolumns = dropemptyrows.dropna(axis = 1) | ||
| # files = ! ls | ||
|
|
||
| Dict = {} |
There was a problem hiding this comment.
Make the variable name more descriptive and lower case. dict is also name for a built-in type in Python and typing library has a class Dict so it's best to avoid using either.
| result = pd.concat([result, newrow], ignore_index = True) | ||
| return result | ||
|
|
||
|
|
No description provided.