Re-format the CNV overview plots in ppt report. issue76 #77
Conversation
marrip
left a comment
marrip
left a comment
There was a problem hiding this comment.
Looks good, we need to test it though meaning merging it into main, getting a latest tag for the docker image and then we can run it at HUS.
One thing, I mentioned earlier, it would be nice if the positioning of the pictures on the slides could be calculated mathematically instead of placing them according to what was empirically determined. But I understand that this would maybe require more rewriting so let's skip it for now.
| pdf_page_image_to_ppt(CNV_overview_plots_pdf,output_ppt_file,A2_to_extract,width_scale=1,height_scale=0.5) | ||
| if(DNA_normal_sampleID != ""): | ||
| pages_to_extract = [2, 5, 6] | ||
| B3_C1_to_extract = [5, 6] |
There was a problem hiding this comment.
just a minor comment here, the variable B3_C1_to_extract does not help me understand what it holds - pages_to_extract was more self-explanatory. May I ask the reasoning behind the change?
There was a problem hiding this comment.
ah, looking at line 1536, it is to make the destinction between A2_to_extract and B3_C1_to_extract. What do A2, B3 and C1 stand for?
There was a problem hiding this comment.
Hi Martin, A2, B3 and C1 are the CNV plot image names in the TSOPPI results. And there will be conditions to extract different plots into the report.
There was a problem hiding this comment.
thanks for the clarification. Maybe you could just add a comment about it to make sure anybody reading your code understands
|
Hi, The layout for the CNV overview plots look good now. And I agree with Martin's comment about "positioning of the pictures on the slides could be calculated mathematically instead of placing them according to what was empirically determined.". If it is too much work it is not needed now, but good to plan for it in the future. |
|
Yes, I agree we should have a plan to format the picture positions in the report slides. |
I am testing some things to do that and maybe I can create a PR with my suggestion - I will probably not have time the next days so for me it is fine to merge these changes into |
This was tested for a while in OUS side and confirmed by the biologist group.