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

New function ppc_km_overlay_grouped() #260

Merged
merged 11 commits into from
Oct 6, 2021

Conversation

fweber144
Copy link
Contributor

I added ppc_km_overlay_grouped(). Unfortunately, the + list(data) trick which is used e.g. in ppc_ecdf_overlay_grouped() cannot be used in ppc_km_overlay_grouped() since if it was used, survival::survfit() would not "know" of the grouping and would not be able to take it into account. The solution I have implemented now is kind of hacky. In particular, it requires to pass the grouping to ppc_km_overlay() which I did via the ellipsis (...). I couldn't think of a better solution, but if you have an idea, please let me know.

I also performed some minor fixes, e.g. correcting a typo in "args-groups.R" and updating the expected results for the "cmdstanr methods work" unit test. Just have a look at my commits and decide which ones you want to use.

@fweber144
Copy link
Contributor Author

fweber144 commented Feb 2, 2021

The failing R-CMD-check on Windows might arise from this "withr" issue.

@codecov-io
Copy link

codecov-io commented Feb 2, 2021

Codecov Report

Merging #260 (38ed79c) into master (ecb1676) will increase coverage by 0.00%.
The diff coverage is 97.18%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #260   +/-   ##
=======================================
  Coverage   98.36%   98.37%           
=======================================
  Files          32       32           
  Lines        4155     4175   +20     
=======================================
+ Hits         4087     4107   +20     
  Misses         68       68           
Impacted Files Coverage Δ
R/ppc-censoring.R 97.18% <97.18%> (+1.94%) ⬆️
R/ppc-intervals.R 99.36% <0.00%> (-0.04%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ecb1676...479fded. Read the comment docs.

@jgabry
Copy link
Member

jgabry commented Oct 6, 2021

Hi @fweber144 thank you for this and really sorry I forgot about reviewing this for so long! I finally have some time to put into bayesplot and I noticed that I had missed this and other PRs when I was super busy with other stuff.

I added ppc_km_overlay_grouped(). Unfortunately, the + list(data) trick which is used e.g. in ppc_ecdf_overlay_grouped() cannot be used in ppc_km_overlay_grouped() since if it was used, survival::survfit() would not "know" of the grouping and would not be able to take it into account. The solution I have implemented now is kind of hacky. In particular, it requires to pass the grouping to ppc_km_overlay() which I did via the ellipsis (...). I couldn't think of a better solution, but if you have an idea, please let me know.

I agree this is a bit hacky but I understand why you went with this option and I'm not sure that I have a better idea. So let's go with this for now and we can always update it later if you or anyone else has an idea for how to improve it.

@codecov-commenter
Copy link

Codecov Report

Merging #260 (ce2d56c) into master (c6919eb) will decrease coverage by 0.00%.
The diff coverage is 97.18%.

❗ Current head ce2d56c differs from pull request most recent head b9f7819. Consider uploading reports for the commit b9f7819 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #260      +/-   ##
==========================================
- Coverage   98.38%   98.37%   -0.01%     
==========================================
  Files          32       32              
  Lines        4199     4184      -15     
==========================================
- Hits         4131     4116      -15     
  Misses         68       68              
Impacted Files Coverage Δ
R/ppc-censoring.R 97.18% <97.18%> (+1.94%) ⬆️
R/mcmc-distributions.R 99.43% <0.00%> (-0.09%) ⬇️
R/mcmc-intervals.R 99.22% <0.00%> (-0.02%) ⬇️
R/helpers-mcmc.R 98.58% <0.00%> (-0.02%) ⬇️
R/mcmc-recover.R 100.00% <0.00%> (ø)
R/mcmc-parcoord.R 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6919eb...b9f7819. Read the comment docs.

@jgabry jgabry merged commit 9561cf8 into stan-dev:master Oct 6, 2021
@fweber144 fweber144 deleted the ppc_censoring_grouped branch October 7, 2021 04:56
@fweber144
Copy link
Contributor Author

No problem, you must have a lot of work with all those R packages. Thanks for merging!

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.

None yet

4 participants