Skip to content

Insert table to ppt#99

Open
marrip wants to merge 18 commits intoInPreD:develop_issue91from
marrip:insert_table_to_ppt
Open

Insert table to ppt#99
marrip wants to merge 18 commits intoInPreD:develop_issue91from
marrip:insert_table_to_ppt

Conversation

@marrip
Copy link
Collaborator

@marrip marrip commented Jan 29, 2026

hey @xiaoliz0 ,

I have started on the refactoring. I have introduced pandas and moved the part that checks the column headers and moves the columns around in an extra function. I am keeping additional headers in the table but I place them to the far right so that they will get ignored with the current setup but I think we should discuss if they should be handled differently. I will continue with the refactoring but please feel free to give feedback and suggestions in the meantime.

@marrip marrip requested a review from xiaoliz0 January 29, 2026 13:40
@marrip marrip self-assigned this Jan 29, 2026
@xiaoliz0
Copy link
Contributor

xiaoliz0 commented Feb 3, 2026

Hi Martin,
Thanks for the contribution to the new function! I am not very confident to review the details with the codes. I did some testings in our environment with your new codes. There is one issue that one column "AF_tumor_DNA" in the tables in report showing wrong format. It should be float I think. This number should be displayed to three decimal places, but now there are many many numbers.

@marrip
Copy link
Collaborator Author

marrip commented Feb 3, 2026

Hi Martin, Thanks for the contribution to the new function! I am not very confident to review the details with the codes. I did some testings in our environment with your new codes. There is one issue that one column "AF_tumor_DNA" in the tables in report showing wrong format. It should be float I think. This number should be displayed to three decimal places, but now there are many many numbers.

Hey Xiaoli, thanks for your feedback! I have added another small function that can basically convert any column into the 2 decimal format that you want. I applied it for AF_tumor_DNA which hopefully fixes the issue you were seeing 🤞

@xiaoliz0
Copy link
Contributor

xiaoliz0 commented Feb 5, 2026

Hi Martin, Thanks for the contribution to the new function! I am not very confident to review the details with the codes. I did some testings in our environment with your new codes. There is one issue that one column "AF_tumor_DNA" in the tables in report showing wrong format. It should be float I think. This number should be displayed to three decimal places, but now there are many many numbers.

Hey Xiaoli, thanks for your feedback! I have added another small function that can basically convert any column into the 2 decimal format that you want. I applied it for AF_tumor_DNA which hopefully fixes the issue you were seeing 🤞

Hi Martin, I did a quick testing today and there is some errors related.
Screenshot_error_PR99

@marrip
Copy link
Collaborator Author

marrip commented Feb 5, 2026

Hi Martin, Thanks for the contribution to the new function! I am not very confident to review the details with the codes. I did some testings in our environment with your new codes. There is one issue that one column "AF_tumor_DNA" in the tables in report showing wrong format. It should be float I think. This number should be displayed to three decimal places, but now there are many many numbers.

Hey Xiaoli, thanks for your feedback! I have added another small function that can basically convert any column into the 2 decimal format that you want. I applied it for AF_tumor_DNA which hopefully fixes the issue you were seeing 🤞

Hi Martin, I did a quick testing today and there is some errors related.

I think I know why this happens. I have introduced a fix. Instead of assuming float it will convert string to float and then do the decimal rounding

@marrip
Copy link
Collaborator Author

marrip commented Feb 5, 2026

I have refactored some more. I am quite happy with the result, we are actually testing smaller parts now and now that they work on their own. Not sure if we should maybe add a test for the whole function insert_table_to_ppt. You could give me your use case and we test that it produces a certain output every time.

Script/PRONTO.py Outdated
cell.text_frame.paragraphs[0].font.size = Pt(font_size)

# add table title
pronto.add_table_title(shapes, table_name, left_h, top_h, width_h, 0.25, 8, print_row_num, slide_idx, total_slides_needed, rows)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to use pronto.add_table_name here? I cannot find this function in the pronto module.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, thanks for pointing that out! I fixed it

@xiaoliz0
Copy link
Contributor

Hi Martin,
Some other errors with further testings in outside.
Screenshot_error_PR99

There are different printing format for this column in the tables in the middle of the report and in the end of the report. In the middle tables it should be printed as float, but in the last table it should be printed as "%" format. These are the previous requests from biology group before.

@marrip marrip force-pushed the insert_table_to_ppt branch from 5d5ae85 to bb2dacc Compare February 13, 2026 12:40
@marrip
Copy link
Collaborator Author

marrip commented Feb 13, 2026

Hi Martin, Some other errors with further testings in outside.

There are different printing format for this column in the tables in the middle of the report and in the end of the report. In the middle tables it should be printed as float, but in the last table it should be printed as "%" format. These are the previous requests from biology group before.

I hope my latest patch fixes this. Again, I would like to maybe use the different inputs for insert_table_to_ppt() as test cases for a unit test. Maybe you could provide me with input cases that you want to be considered so I can make sure the function covers those. This way you would encounter less of these cases to fail 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants