Skip to content
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

unify coordinates and visits_zero_index #366

Merged
merged 5 commits into from
Nov 1, 2023

Conversation

clarkliming
Copy link
Collaborator

@clarkliming clarkliming commented Oct 31, 2023

close #110

current implementation unifies coordinates and visits.

There is an extra data conversion so it is expected there is a small cost.
Average computation time for FEV1 ~ ARMCD + us(AVISIT|USUBJID) on fev_data decrease from 37.53347 ms to 37.89274 ms (but still very small and neglectable I think)

In addition, the select matrix is removed but instead use "subset_matrix" functionality

@clarkliming clarkliming marked this pull request as draft October 31, 2023 02:38
@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023

badge

Code Coverage Summary

Filename                    Stmts    Miss  Cover    Missing
------------------------  -------  ------  -------  -------------------------------
R/between-within.R             59       0  100.00%
R/component.R                  67       0  100.00%
R/cov_struct.R                 97       1  98.97%   407
R/empirical.R                   7       0  100.00%
R/fit.R                       223      11  95.07%   186-189, 407, 450-453, 468, 498
R/interop-emmeans.R            39       0  100.00%
R/interop-parsnip.R            59       1  98.31%   12
R/kenwardroger.R               92       2  97.83%   41, 63
R/mmrm-methods.R              120       0  100.00%
R/residual.R                    8       8  0.00%    10-31
R/satterthwaite.R             116      12  89.66%   238-249
R/testing.R                    55       4  92.73%   29, 31, 70, 72
R/tidiers.R                    72       2  97.22%   46-47
R/tmb-methods.R               287       3  98.95%   277-278, 338
R/tmb.R                       287       0  100.00%
R/utils-formula.R              27       0  100.00%
R/utils-nse.R                  16       0  100.00%
R/utils.R                     124      10  91.94%   276-286
R/zzz.R                        65      19  70.77%   7-17, 50-55, 85, 113, 133
src/chol_cache.h               63       0  100.00%
src/covariance.h              101       1  99.01%   177
src/derivatives.h             126       0  100.00%
src/empirical.cpp              72       0  100.00%
src/exports.cpp                47       0  100.00%
src/jacobian.cpp               47       1  97.87%   54
src/kr_comp.cpp                56       0  100.00%
src/mmrm.cpp                   76       0  100.00%
src/predict.cpp                93       0  100.00%
src/test-chol_cache.cpp        58       5  91.38%   9, 18, 26, 55, 62
src/test-covariance.cpp       123       5  95.93%   9, 29, 40, 61, 72
src/test-derivatives.cpp      108       7  93.52%   44, 53, 62, 85, 94, 106, 124
src/test-utils.cpp            195       7  96.41%   9, 16, 24, 34, 44, 57, 119
src/testthat-helpers.h         15       5  66.67%   36-37, 41, 50, 53
src/utils.h                    84       0  100.00%
TOTAL                        3084     104  96.63%

Diff against main

Filename                    Stmts    Miss  Cover
------------------------  -------  ------  --------
R/tmb.R                        -3       0  +100.00%
src/chol_cache.h               -3       0  +100.00%
src/derivatives.h              -8       0  +100.00%
src/empirical.cpp              -1       0  +100.00%
src/jacobian.cpp               -1       0  -0.04%
src/kr_comp.cpp                -1       0  +100.00%
src/mmrm.cpp                   -1       0  +100.00%
src/predict.cpp                -2       0  +100.00%
src/test-derivatives.cpp       -3       0  -0.18%
src/test-utils.cpp             +5      +2  -0.96%
src/utils.h                    +2       0  +100.00%
TOTAL                         -16      +2  -0.08%

Results for commit: 6d0c939

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

@github-actions
Copy link
Contributor

github-actions bot commented Oct 31, 2023

Unit Tests Summary

       1 files       43 suites   25s ⏱️
   462 tests    425 ✔️ 37 💤 0
1 819 runs  1 777 ✔️ 42 💤 0

Results for commit 6d0c939.

♻️ This comment has been updated with latest results.

@danielinteractive
Copy link
Collaborator

Thanks @clarkliming , does this then also close #270 ?

Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

nice simplification, thanks @clarkliming !

src/derivatives.h Outdated Show resolved Hide resolved
src/test-utils.cpp Show resolved Hide resolved
clarkliming and others added 2 commits November 1, 2023 13:28
Copy link
Collaborator

@danielinteractive danielinteractive left a comment

Choose a reason for hiding this comment

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

very nice, thanks!!

@danielinteractive danielinteractive marked this pull request as ready for review November 1, 2023 07:55
@clarkliming
Copy link
Collaborator Author

maybe we can wait for CRAN 0.3.5 release accepted and then merge

@danielinteractive
Copy link
Collaborator

I would merge now. this will not affect the release, it will be part of next version

@clarkliming clarkliming merged commit 38e4aa3 into main Nov 1, 2023
@clarkliming clarkliming deleted the 110_improve_handling_corrdinates branch November 1, 2023 08:23
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.

Improve handling of visits_zero_inds and coordinates
2 participants