Skip to content

Optimize .compress_mat #353

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Optimize .compress_mat #353

wants to merge 14 commits into from

Conversation

Melkiades
Copy link
Contributor

Fixes #352

@Melkiades Melkiades added the enhancement New feature or request label Jun 12, 2025
Copy link
Contributor

github-actions bot commented Jun 12, 2025

Unit Tests Summary

  1 files    8 suites   13s ⏱️
 55 tests  55 ✅ 0 💤 0 ❌
416 runs  416 ✅ 0 💤 0 ❌

Results for commit 22db7fc.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jun 12, 2025

badge

Code Coverage Summary

Filename             Stmts    Miss  Cover    Missing
-----------------  -------  ------  -------  -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/format_value.R       262       5  98.09%   110, 216, 255, 523, 531
R/generics.R           125      12  90.40%   148, 286-290, 492, 504, 535, 565, 673, 686, 707-714
R/labels.R              55       7  87.27%   51, 57, 66, 107, 133, 142, 146
R/matrix_form.R        695      42  93.96%   134, 174, 405, 525-526, 618, 628-631, 649, 680, 770-771, 785-790, 820-823, 883-884, 973-974, 1001, 1029, 1081, 1243, 1279, 1282-1286, 1336, 1384, 1387, 1391
R/mpf_exporters.R      287      28  90.24%   2, 102-112, 157, 193, 238, 241, 246, 426-432, 436, 439, 443, 491, 570
R/page_size.R           42       1  97.62%   219
R/pagination.R         763      56  92.66%   327-330, 435-450, 540, 595, 600, 641, 679-690, 766, 878-879, 901-910, 1051, 1054, 1261, 1298-1302, 1319-1327, 1408, 1548-1549, 1565-1566, 1580-1581
R/tostring.R           783      66  91.57%   88, 296, 351, 421, 454, 462, 498, 555-558, 594, 660-663, 669-673, 676-679, 686-691, 774-775, 915-916, 981-988, 1038-1042, 1111, 1164, 1183-1187, 1198, 1216, 1233, 1248, 1346, 1389, 1434, 1520, 1559, 1613, 1620
R/utils.R                3       0  100.00%
R/zzz.R                 17       6  64.71%   28-33
TOTAL                 3032     223  92.65%

Diff against main

Filename           Stmts    Miss  Cover
---------------  -------  ------  -------
R/matrix_form.R       +3      +1  -0.12%
R/pagination.R        -1      +4  -0.53%
TOTAL                 +2      +5  -0.16%

Results for commit: 22db7fc

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Copy link
Contributor

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
pagination 💚 $7.00$ $-1.10$ $-1$ $0$ $0$ $0$

Results for commit 0b09cae

♻️ This comment has been updated with latest results.

@shajoezhu
Copy link
Contributor

hi @Melkiades , can you check if there is any downstream breaking changes in rtables/rlistings/scda.test/chevron please. thank you!

@shajoezhu
Copy link
Contributor

@Melkiades
Copy link
Contributor Author

see https://code.roche.com/search?search=cpp&nav_source=navbar&project_id=198744&group_id=14397&search_code=true&repository_ref=main, rlistings fails?

Yes there may be issues downstream as some objects are supposed differently. I am checking downstream now.

@Melkiades
Copy link
Contributor Author

@shajoezhu @BFalquet ready to review

@Melkiades
Copy link
Contributor Author

@edelarua @ayogasekaram could you take a look if you have time? It passes all tests downstream (revedep)

Copy link
Contributor

@edelarua edelarua left a comment

Choose a reason for hiding this comment

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

This looks great!!

@shajoezhu
Copy link
Contributor

hi @Melkiades , can you do manual cicd checks in downstream packages, the reverse dependency check github action is not working at the the moment. @pawelru is still repairing this.

@shajoezhu
Copy link
Contributor

hi @Melkiades , can you do manual cicd checks in downstream packages, the reverse dependency check github action is not working at the the moment. @pawelru is still repairing this.

i think the easiest is to create new PRs in rtables/rlistings/chevron/scda.test repos,

adding the following to rtables/rlistings

Remotes:
    insightsengineering/formatters@352_mclapply_pagination@main

and chevron and scda.test need to add the branches for rtables and rlistings to trigger the test. thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Faster pagination with mclapply
3 participants