search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
722 stars 38 forks source link

[REVIEW]: CVtreeMLE: Efficient Estimation of Mixed Exposures using Data Adaptive Decision Trees and Cross-Validated Targeted Maximum Likelihood Estimation in R #4181

Closed whedon closed 1 year ago

whedon commented 2 years ago

Submitting author: !--author-handle-->@blind-contours<!--end-author-handle-- (David McCoy) Repository: https://github.com/blind-contours/CVtreeMLE Branch with paper.md (empty if default branch): Version: v1.0-joss Editor: !--editor-->@osorensen<!--end-editor-- Reviewers: @GaryBAYLOR, @cpalmer718, @wleoncio Archive: 10.5281/zenodo.7651354

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

Status

status

Status badge code:

HTML: <a href="https://joss.theoj.org/papers/e3f631afd3bbcd60ed5d965ce26a39b0"><img src="https://joss.theoj.org/papers/e3f631afd3bbcd60ed5d965ce26a39b0/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/e3f631afd3bbcd60ed5d965ce26a39b0/status.svg)](https://joss.theoj.org/papers/e3f631afd3bbcd60ed5d965ce26a39b0)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@GaryBAYLOR & @cpalmer718, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @osorensen know.

✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨

Review checklist for @GaryBAYLOR

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @cpalmer718

✨ Important: Please do not use the Convert to issue functionality when working through this checklist, instead, please open any new issues associated with your review in the software repository associated with the submission. ✨

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 2 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @GaryBAYLOR, @cpalmer718 it looks like you're currently assigned to review this paper :tada:.

:warning: JOSS reduced service mode :warning:

Due to the challenges of the COVID-19 pandemic, JOSS is currently operating in a "reduced service mode". You can read more about what that means in our blog post.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands

For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:

@whedon generate pdf
whedon commented 2 years ago

Wordcount for paper.md is 1248

whedon commented 2 years ago 
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.26 s (186.7 files/s, 31606.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
R                               39            832           1214           2592
HTML                             1             35              2           1020
Markdown                         3            125              0            872
TeX                              2             55              0            631
Rmd                              2            241            425            180
YAML                             2             12              0             57
-------------------------------------------------------------------------------
SUM:                            49           1300           1641           5352
-------------------------------------------------------------------------------

Statistical information for the repository '3c477f102d988190661a0dcd' was
gathered on 2022/02/18.
No commited files with the specified extensions were found.
whedon commented 2 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1515/ijb-2015-0013 is OK
- 10.2202/1557-4679.1217 is OK
- 10.1111/j.1541-0420.2011.01685.x is OK
- 10.1002/sim.5907 is OK
- 10.1007/978-3-319-65304-4_14 is OK
- 10.1007/978-1-4419-9782-1 is OK
- 10.1007/978-3-319-65304-4 is OK
- 10.2202/1557-4679.1356 is OK
- 10.1111/rssb.12362 is OK
- 10.1111/biom.13375 is OK
- 10.21105/joss.02526 is OK
- 10.5281/zenodo.1342293 is OK
- 10.5281/zenodo.3558313 is OK
- 10.5281/zenodo.3698329 is OK

MISSING DOIs

- None

INVALID DOIs

- None
whedon commented 2 years ago

:point_right::page_facing_up: Download article proof :page_facing_up: View article proof on GitHub :page_facing_up: :point_left:

GaryBAYLOR commented 2 years ago

@osorensen I reviewed the paper and have the following comments. I will take another look after these issues are resolved. Thanks!

Why two licenses https://github.com/blind-contours/CVtreeMLE/issues/2 The package cannot be found in CRAN https://github.com/blind-contours/CVtreeMLE/issues/3 Improve function documentation https://github.com/blind-contours/CVtreeMLE/issues/4 Lightweight editing of the JOSS paper https://github.com/blind-contours/CVtreeMLE/issues/5 Comments on the paper https://github.com/blind-contours/CVtreeMLE/issues/6 Remove R code that are no longer needed https://github.com/blind-contours/CVtreeMLE/issues/7 Comments on README file https://github.com/blind-contours/CVtreeMLE/issues/8 Non reproducible results https://github.com/blind-contours/CVtreeMLE/issues/9

osorensen commented 2 years ago

Thanks @GaryBAYLOR!

lightning-auriga commented 2 years ago

Hi @osorensen I'm ready to post the review, but I can't edit the checklist message. I'm logged in, and I tried the invitation link but it said it couldn't find an invitation. Can you reinvite me? I apologize if this was (quite likely) because I waited too long to click the link.

osorensen commented 2 years ago

Thanks for reaching out @cpalmer718. I hope @openjournals/dev can help us with this issue.

osorensen commented 2 years ago

Or perhaps you could try running the following comment in this issue, @cpalmer718: @editorialbot generate my checklist. This should generate a new checklist, which you can edit.

lightning-auriga commented 2 years ago

Review checklist for @cpalmer718

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

lightning-auriga commented 2 years ago

@osorensen oooh excellent, thank you

osorensen commented 2 years ago

Glad it worked @cpalmer718. Then just use this checklist rather than the one at the top.

lightning-auriga commented 2 years ago

Extended comments/issue links:

lightning-auriga commented 2 years ago

Thanks for having me @osorensen et al.! Hope it goes well, and I'd be happy to take a second look.

osorensen commented 2 years ago

Thanks a lot for your review, @cpalmer718. Please notify us when these issues have been resolved, @blind-contours, and don't hesitate to reach out if I can be of any help.

blind-contours commented 2 years ago

Hi Team, thank you so much for your review and sorry for my delayed response. I'm preparing for my qualifying exam currently. I will begin making edits based on your suggestions soon. Again, thank you for the thorough review, this was extremely helpful!

osorensen commented 2 years ago

@blind-contours, could you please update us on how it's going with addressing the issues raised by the reviewers?

osorensen commented 2 years ago

@blind-contours, can you please update us on how it is going with addressing the points raised by the reviewers?

osorensen commented 2 years ago

@openjournals/joss-eics I have not been able to get in touch with the submitting author @blind-contours, despite repeated attempts. The last response is on 17 March and none of the issues opened by the reviewers in the source repository appear to have been addressed. Could you please consider if this submission should be rejected, with an opportunity to resubmit? I have also searched extensively for the author's e-mail address, but not been able to find it.

Furthermore, the manuscript claims that "The CVtreeMLE package has been made publicly available both via GitHub and the Comprehensive R Archive Network." However, I am not able to find the package on the Comprehensive R Archive Network.

blind-contours commented 2 years ago

@osorensen -- sorry for the delay. I've started addressing the issues made by @cpalmer718. My email address is david_mccoy@berkeley.edu, I will add it to the readme and update the link to my bio page. I wasn't able to get the package on CRAN because the SL3 dependency is not on CRAN so I am removing that section of the manuscript and updating the READme. Thank you for your review of this package, I will be responding to issues this week.

osorensen commented 2 years ago

Thanks @blind-contours. Please keep us updated about the progress and feel free to reach out to me whenever you have any questions .

osorensen commented 2 years ago

@blind-contours, could you please update us on how it is going addressing the issues opened by the reviewers? It would also be nice if you could give an approximate estimate of how long you expect it will take to resolve the issues.

blind-contours commented 2 years ago

Hi @osorensen, I have responded to issue #5 and #6 in regards to the JOSS paper submission. Generally, I have added details to elaborate on more abstruse terms and moved parts of different sections around to better match organization. Hopefully these edits meet the reviewers comments but please let me know if there are any other suggestions. The current paper.md is the most updated draft.

blind-contours commented 2 years ago

Issues #1 and #3 have been resolved - this is regarding CVtreeMLE not being available on CRAN. I've simply removed this from the documentation and download is only available off github as the package currently relies on SL3 for ensemble machine learning. In the future, I can update code to use tidymodels or HAL9001 nonparametric estimators which will allow for CRAN submission but for now this is not available given SL3 is not on CRAN.

blind-contours commented 2 years ago

I am currently cleaning the existing code to pass linting tests and updating documentation to meet reviewer comments #7, #4, and #14. I expect all issues to be resolved by August 1st.

osorensen commented 2 years ago

Thanks @blind-contours! It's absolutely fine that this takes some time, but please give a new update on the progress in the beginning of August.

blind-contours commented 2 years ago

@osorensen et. al., I have made significant changes to the package in response to the JOSS issues page. I have given responses to each and made updates to the package accordingly. I'm still making some small changes but for the most part I believe the new updates are a major improvement in the package given the issues raised by reviewers. This is the first major package I have created so I really appreciate the thorough review. Please let me know what additional changes you would like to see after reviewing what I have already done. Thanks again - really appreciate the time and effort.

osorensen commented 2 years ago

Thanks a lot for the work, @blind-contours. Could you please update your review checklists based on these latest modifications, @cpalmer718 and @GaryBAYLOR? Please let me know if you have any further questions.

osorensen commented 2 years ago

👋 @cpalmer718 and @GaryBAYLOR, could you please consider whether the updates made by @blind-contours address your points raised in the reviews, and update your review checklists accordingly?

lightning-auriga commented 2 years ago

Hi @osorensen @blind-contours the package is much improved, and substantial effort has been put into addressing my comments. I'll summarize the following remaining notes:

These are all quite minor, and beyond that I am content.

osorensen commented 2 years ago

Thanks @cpalmer718!

osorensen commented 2 years ago

On the occasion, I'd also like to ask @blind-contours about the prospects of submitting this package to CRAN. Although not a strict requirement for publishing in JOSS, it definitely would be an advantage.

blind-contours commented 2 years ago

Thank you @cpalmer718! As per your first point, I'm looking to reduce the bloat. I am still learning the intricacies of namespaces - are there particular packages you see that are only used via library()? I'll work on your additional comments. Really appreciate your thorough review! It's been a great learning experience.

blind-contours commented 2 years ago

I would like to get this package on CRAN. The major issue is the ensemble machine learning used to estimate the data adaptive parameter and the nuisance parameters. Because SL3 - the ensemble machine learning package I use, is not on CRAN, and I can't really make it optional while keeping the statistical properties of the estimates, I would need to use a package that is on CRAN and does a type of Super Learning. I believe I can substitute SL3 for tidymodels. So my plan is, once this package is on JOSS (hopefully) to create a new branch for development of transferring the machine learning functions to use tidymodels - this would require a lot of restructuring of the package but would make it available on CRAN.

osorensen commented 2 years ago

@blind-contours, I'm the editor and not the reviewer here, but would like to point out that the following lines in your NAMESPACE import all functions from ggplot2, partykit, and sl3. I think this is @cpalmer718's point here.

import(ggplot2)
import(partykit)
import(sl3)

Inside the code, however, I see that you are using the namespace qualifier when calling ggplot2 functions, e.g., in these lines from plot_marginal_results.R. Presumably the same applies to partykit and sl3 as well.

      plot <- ggplot2::ggplot(
        marg_data,
        ggplot2::aes_string(
          x = "`Marginal ATE`", y = "fold", color = "fold",
          xmin = "`Lower CI`", xmax = "`Upper CI`", label = "`Comparison`"
        )
      ) +
        ggplot2::facet_wrap(~Type) +
        ggplot2::geom_errorbarh(size = line_size) +
        ggplot2::geom_point(size = point_size) +
        ggplot2::geom_vline(xintercept = 0, alpha = .25, linetype = "dotted",
                            size = line_size) +
        ggplot2::labs(x = "ATE", y = "Fold", color = "") +
        ggplot2::ggtitle(title) +
        ggplot2::theme_classic() +
        ggplot2::theme(text = text_theme, axis.text = axis_text_theme,
                       legend.position = "none") +
        ggplot2::geom_text(size = 5, hjust = hjust, vjust = 0,
                           colour = "#3C3C3C", nudge_x = 0, nudge_y = 0.0)

This is unnecessary. You could either:

(1) The best option:

Remove the following line

#' @import ggplot2

Then run devtools::document() or whatever you're using, and make sure import(ggplot2) is no longer in your NAMESPACE.

(2) Not a good option

... but the whole reason for having @import ggplot2 there in the first place. Remove the ggplot2:: qualifier from all of your source code.

In summary, whenever you're using pkg::function in your source code, you should NOT import that function, or import the package (which means important ALL functions in the package). In these cases pkg should only be listed in your DESCRIPTION. The point of important is to save oneself from writing pkg:: everywhere, which can make the source code more readable if you're reusing the same function a large number of times.

osorensen commented 2 years ago

Another example I found while browsing source code. In simulate_mixture_cube.R you have the line

    dplyr::mutate(rcat = Hmisc::rMultinom(probs, 1))

but you also write 

#' @importFrom Hmisc rMultinom

Again, since you use the qualifier Hmisc::, the @importFrom should be removed here.

blind-contours commented 2 years ago

@osorensen - thanks so much for the clarification. I think I get it now and will do more research. I'll remove these packages from the namespace.

GaryBAYLOR commented 2 years ago

@blind-contours I have updated some issues as well as adding a few new issues I detected. I will take time to look at issue #6 and #8 I raised previously given you have updated them.

osorensen commented 2 years ago

@GaryBAYLOR, could you please update us on how it's going addressing the issues raised by the reviewers?

blind-contours commented 2 years ago

I’ve made some big updates to the package. Currently passes everything with no notes, warnings or errors but it appears something is failed the check with GitHub Linux which I’m trying to figure out. I think it’s just the parallel processing in the vignette on Linux. I think I’ve responded to everything else. Let me know if I can do anything else.

osorensen commented 2 years ago

Thanks for responding @blind-contours, and sorry @GaryBAYLOR that I wrongly tagged you.

osorensen commented 2 years ago

@blind-contours, if you think most of the issues raised by the reviewers have been resolved, please tag them with @username in the respective issues in the source repository.

osorensen commented 1 year ago

👋 @GaryBAYLOR and @cpalmer718, could you please check if the points raised in your reviews have now been addressed?

lightning-auriga commented 1 year ago

@osorensen There are still no contributing guidelines and the corresponding link in the readme is broken. I'm fine with the paper at this point, effort has obviously been put in here.

osorensen commented 1 year ago

Thanks a lot @cpalmer718.

@blind-contours, could you please fix the two remaining issues pointed out by @cpalmer718 and report here when done?

osorensen commented 1 year ago

Thanks a lot @cpalmer718.

@blind-contours, could you please fix the two remaining issues pointed out by @cpalmer718 and report here when done?

@blind-contours, could you please update us on this one?

@GaryBAYLOR could you please update us on how it's going with your review?

blind-contours commented 1 year ago

@osorensen I have added contributing guidelines and the corresponding link now maps to my website. Package should pass with no notes, warnings, or errors. I'm still making small changes to the vignette and general code streamlining but I think I've responded to all the comments/suggestions at this point. Thank you!

osorensen commented 1 year ago

Thanks @blind-contours. @cpalmer718 could you please let us know if this addresses you concerns, and update the review checklist if it does?

osorensen commented 1 year ago

👋 @SaranjeetKaur @calebsjin, would any of you be willing to review this submission for JOSS? We carry out our checklist-driven reviews here in GitHub issues and follow these guidelines: https://joss.readthedocs.io/en/latest/review_criteria.html

The review of this submission has been going on for a while and the authors has addressed a number of issues, but we have lost contact with one of the reviewers. We would hence very much appreciate the opinion of one or two additional reviewers.