rewriting_trending_from_mip6_into_mip5 for delivery reports #25
Conversation
patrikgrenfeldt
changed the title
Pull Request Test Coverage Report for Build 387
💛 - Coveralls |
trailblazer/mip/trending.py
Outdated
| ### Define output dict | ||
| outdata = { | ||
| 'analysis_sex': {}, | ||
| 'at_dropout': {}, |
There was a problem hiding this comment.
We could comment this one out
There was a problem hiding this comment.
This is an important qc-metric to trend as was decided previously. Are you adding this info from some other source?
I think it is bad practise to leave lines of code that are not used in a "commented form". It clutteres the code and makes it more complicated than it is. Removing the unused line is prefered and then adding it in when required. In this case I do not really understand why you want to comment it out as you can just ignore this key when parsing the output in the downstream process (if it is not ready for the "at_dropout"), but I am not up to speed on what you are planning here.
There was a problem hiding this comment.
Ok, now I see you are just trying to get it to be compatible with the mip5.0 in production, right?
There was a problem hiding this comment.
... like the title said ;)
trailblazer/mip/trending.py
Outdated
| 'at_dropout': {}, | ||
| 'family': None, | ||
| 'duplicates': {}, | ||
| 'gc_dropout': {}, |
There was a problem hiding this comment.
This is an important qc-metric to trend as was decided previously. Are you adding this info from some other source?
trailblazer/mip/trending.py
Outdated
| 'duplicates': {}, | ||
| 'gc_dropout': {}, | ||
| 'genome_build': None, | ||
| 'insert_size_standard_deviation': {}, |
There was a problem hiding this comment.
This is an important qc-metric to trend as was decided previously. Are you adding this info from some other source?
trailblazer/mip/trending.py
Outdated
| 'genome_build': None, | ||
| 'insert_size_standard_deviation': {}, | ||
| 'mapped_reads': {}, | ||
| 'median_insert_size': {}, |
There was a problem hiding this comment.
This is an important qc-metric to trend as was decided previously. Are you adding this info from some other source?
trailblazer/mip/trending.py
Outdated
| dict: parsed data | ||
| """ | ||
|
|
||
| ### Define output dict |
There was a problem hiding this comment.
What's up with these triple #?
There was a problem hiding this comment.
It's just a way of commenting what the next block of code is handling. If you have another system in place in trailblazer feel free to modify accordingly.
There was a problem hiding this comment.
Python has a style guide which we are following: https://www.python.org/dev/peps/pep-0008/
e.g. Pycharm automatically highlights any deviations from pep-8.
trailblazer/mip/trending.py
Outdated
| 'sample_ids': [], | ||
| } | ||
| ### Config | ||
| ## Parse raw mip_config into dict |
There was a problem hiding this comment.
What's up with these dual #?
trailblazer/mip/trending.py
Outdated
| ## Add mip version | ||
| outdata['mip_version'] = sampleinfo_data['version'] | ||
|
|
||
| ## Add mip version |
|
All requested changes performed, what do you think of it now? |
|
Wow, You really went all in for utility subs, 😁. Very nice and clear! 👍 |
|
Let's merge and 🚢 |
…azer