Closed mahaalbashir closed 1 year ago
Merging #145 (82c6cdc) into main (40169b5) will decrease coverage by
0.47%
. The diff coverage is95.29%
.:exclamation: Current head 82c6cdc differs from pull request most recent head 3bab95a. Consider uploading reports for the commit 3bab95a to get more accurate results
@@ Coverage Diff @@
## main #145 +/- ##
===========================================
- Coverage 100.00% 99.53% -0.47%
===========================================
Files 7 7
Lines 783 866 +83
===========================================
+ Hits 783 862 +79
- Misses 0 4 +4
Files Changed | Coverage Δ | |
---|---|---|
acro/record.py | 98.58% <78.57%> (-1.42%) |
:arrow_down: |
acro/acro.py | 99.64% <98.59%> (-0.36%) |
:arrow_down: |
@rpreen do you think I should move some of the functions to for example utils.py to solve the (too-many-lines) error?
@rpreen do you think I should move some of the functions to for example utils.py to solve the (too-many-lines) error?
The acro.py
module needs to be broken up, but I think it's more than just moving things to utils.py
. In fact, some of the functions in utils.py
are really specific to the pandas crosstab
and pivot_table
functions and aren't really utilities at all. Really, we want one module with the pandas functions in, another for the statsmodels functions, and another for the survival analysis stuff. The thing is that we probably still want to maintain all the functions within one Acro
class, so I'm not entirely sure the best way to do it; maybe using multiple inheritance or some other way... Maybe there is another architecture that still has a nice usability for the researcher; it needs some thought.
I think multiple inheritance is probably the way to go. Are you comfortable trying to break it up that way?
That is a good idea. I can try doing that. Is it better to check this pull request for the survival analysis first and then do the reformatting separately?
This is not the full implementation of the kaplan-meier. In this version:
What should be added:
Questions: