search

openjournals / joss-reviews

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

[REVIEW]: FrESCO: Framework for Exploring Scalable Computational Oncology #5345

Closed editorialbot closed 1 year ago

editorialbot commented 1 year ago

Submitting author: !--author-handle-->@aspannaus<!--end-author-handle-- (Adam Spannaus) Repository: https://github.com/DOE-NCI-MOSSAIC/FrESCO Branch with paper.md (empty if default branch): JOSS Version: v0.2.4 Editor: !--editor-->@jmschrei<!--end-editor-- Reviewers: @gabeerion, @anupamajha1 Archive: 10.5281/zenodo.8325993

Status

status

Status badge code:

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

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

@gabeerion & @SimonBiggs, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review. First of all you need to run this command in a separate comment to create the checklist:

@editorialbot generate my checklist

The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @jmschrei 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 ✨

Checklists

📝 Checklist for @gabeerion

📝 Checklist for @anupamajha1

editorialbot commented 1 year ago

Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks.

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

@editorialbot commands

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

@editorialbot generate pdf
editorialbot commented 1 year ago 
Software report:

github.com/AlDanial/cloc v 1.88  T=0.49 s (57.6 files/s, 12989.1 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          18           1134           1133           3432
Markdown                         2             35              0            212
TeX                              2             41             36            162
YAML                             3             12             16            103
JSON                             3              0              0              3
-------------------------------------------------------------------------------
SUM:                            28           1222           1185           3912
-------------------------------------------------------------------------------

gitinspector failed to run statistical information for the repository
editorialbot commented 1 year ago

Wordcount for paper.md is 842

editorialbot commented 1 year ago

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

jmschrei commented 1 year ago

Howdy @gabeerion and @SimonBiggs!

Thanks for agreeing to review this submission.

The process for conducting a review is outlined above. Please run the command shown above to have @editorialbot generate your checklist, which will give a step-by-step process for conducting your review. Please check the boxes during your review to keep track, as well as make comments in this thread or open issues in the repository itself to point out issues you encounter. Keep in mind that our aim is to improve the submission to the point where it is of high enough quality to be accepted, rather than to provide a yes/no decision, and so having a conversation with the authors is encouraged rather than providing a single review post at the end of the process.

Here are the review guidelines: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html And here is a checklist, similar to above: https://joss.readthedocs.io/en/latest/review_checklist.html

Please let me know if you encounter any issues or need any help during the review process, and thanks for contributing your time to JOSS and the open source community!

SimonBiggs commented 1 year ago

Review checklist for @SimonBiggs

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

editorialbot commented 1 year ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- Errored finding suggestions for "Combating Label Noise in Deep Learning using Abste...", please try later
- 10.1371/journal.pone.0232840 may be a valid DOI for title: Using case-level context to classify cancer pathology reports
- Errored finding suggestions for "Automatic extraction of cancer registry reportable...", please try later
- Errored finding suggestions for "PyTorch: An Imperative Style, High-Performance Dee...", please try later
- Errored finding suggestions for "Pyhealth: A python library for health predictive m...", please try later
- 10.21105/joss.04943 may be a valid DOI for title: FuseMedML: a framework for accelerated discovery in machine learning based biomedicine
- Errored finding suggestions for "Med7: A transferable clinical natural language pro...", please try later
- Errored finding suggestions for "Clinical deployment environments: Five pillars of ...", please try later
- Errored finding suggestions for "ECP-Candle", please try later

INVALID DOIs

- https://doi.org/10.1016/j.artmed.2019.101726 is INVALID because of 'https://doi.org/' prefix
gabeerion commented 1 year ago

Review checklist for @gabeerion

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

jmschrei commented 1 year ago

Hi @SimonBiggs @gabeerion, how are the reviews coming?

SimonBiggs commented 1 year ago

Hi @jmschrei,

Not yet begun. I'm hoping to be able to earmark some time next week.

gabeerion commented 1 year ago

@jmschrei actively working on my review over last week and the coming one, thanks!

SimonBiggs commented 1 year ago

Hi @aspannaus,

I was hoping to be able to report an issue, however your issue list is locked down in such a way that I cannot create an account through the web interface.

Would it be possible for you to make a fork of this repository on GitHub where I can access the code without needing an ornl.gov account?

image

image

aspannaus commented 1 year ago

Hi @SimonBiggs,

Let me take a look at the permissions. Someone outside of the organization was able to access the repo earlier this week, so I'm not sure what the issue is. I'll also look into the possibility of creating an open fork on Github. Thanks

I created a fork at https://code.ornl.gov/3t6/fresco that should be completely open. Check it out and let me know if you encounter any issues.

SimonBiggs commented 1 year ago

Hi @aspannaus,

I can confirm that cloning is now working. But I cannot raise an issue there either:

image

aspannaus commented 1 year ago

Hi @SimonBiggs,

Apologies for the continued issue. We've got an internal ticket filed to sort this out; I'll post here once I have some resolution.

Update: we're going to move the repo over to GitHub, but this won't be instantaneous. I'll update here with the new url. Thanks

aspannaus commented 1 year ago

Hi @SimonBiggs, @gabeerion and @jmschrei,

We've moved the repo over to: https://github.com/DOE-NCI-MOSSAIC/FrESCO/tree/JOSS to address the issue noted above about not being able to create a git issue without registering.

Thanks again for your patience.

jmschrei commented 1 year ago

@SimonBiggs @gabeerion how are the reviews coming?

SimonBiggs commented 1 year ago

@jmschrei, just waiting on the following before I continue:

We will finish internal testing with the reorganization to make the repo pip-installable and push the changes early next week.

https://github.com/DOE-NCI-MOSSAIC/FrESCO/issues/1#issuecomment-1554681881

aspannaus commented 1 year ago

Just posting here for completeness, we updated the repository based on @gabeerion comments and suggestions.

https://github.com/DOE-NCI-MOSSAIC/FrESCO/issues/1#issuecomment-1561201091

SimonBiggs commented 1 year ago

Thanks @aspannaus,

I took a few small bites off the review this morning and have left two PRs on your repo for your review.

Cheers, Simon

aspannaus commented 1 year ago

Hey @SimonBiggs,

Thanks for taking a look, we'll start working on the PRs.

aspannaus commented 1 year ago

Hi @SimonBiggs and @gabeerion,

Just checking in on the status of the review, if there's anything else you need from us.

Thanks!

gabeerion commented 1 year ago

Hi, so sorry for the delay! I appreciate the ping and will work on this again tomorrow.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: aspannaus @.> Sent: Monday, June 26, 2023 12:52:48 PM To: openjournals/joss-reviews @.> Cc: Gabriel Erion @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: FrESCO: Framework for Exploring Scalable Computational Oncology (Issue #5345)

Hi @SimonBiggshttps://github.com/SimonBiggs and @gabeerionhttps://github.com/gabeerion,

Just checking in on the status of the review, if there's anything else you need from us.

Thanks!

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5345#issuecomment-1607857570, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA2QE6PWCSUCUQ3WFDVAH33XNG46BANCNFSM6AAAAAAWU3YZIQ. You are receiving this because you were mentioned.Message ID: @.***>

SimonBiggs commented 1 year ago

Hi @jmschrei,

The repository has changed for this review. Does that need to be updated here?

jmschrei commented 1 year ago

Yes. What is the new repo?

jmschrei commented 1 year ago

@SimonBiggs @gabeerion can I get an update on your progress?

SimonBiggs commented 1 year ago

Yes. What is the new repo?

https://github.com/DOE-NCI-MOSSAIC/FrESCO

SimonBiggs commented 1 year ago

can I get an update on your progress?

Keep going to step through the checklist and hitting issues.

jmschrei commented 1 year ago

@gabeerion how is progress going? Can we get this wrapped up soon?

jmschrei commented 1 year ago

@SimonBiggs do you have any active issues that the authors need to address?

jmschrei commented 1 year ago

@editorialbot set https://github.com/DOE-NCI-MOSSAIC/FrESCO as repository

editorialbot commented 1 year ago

Done! repository is now https://github.com/DOE-NCI-MOSSAIC/FrESCO

gabeerion commented 1 year ago

Thanks for the reminder - I am working through it but a bit slowly due to other work. I think I can solidly say I'll be done within a week.

Get Outlook for Androidhttps://aka.ms/AAb9ysg

From: Jacob Schreiber @.> Sent: Wednesday, July 5, 2023 7:13:35 PM To: openjournals/joss-reviews @.> Cc: Gabriel Erion @.>; Mention @.> Subject: Re: [openjournals/joss-reviews] [REVIEW]: FrESCO: Framework for Exploring Scalable Computational Oncology (Issue #5345)

@gabeerionhttps://github.com/gabeerion how is progress going? Can we get this wrapped up soon?

— Reply to this email directly, view it on GitHubhttps://github.com/openjournals/joss-reviews/issues/5345#issuecomment-1622661509, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AA2QE6NEAHMPWPS4TV3JM43XOXYJ7ANCNFSM6AAAAAAWU3YZIQ. You are receiving this because you were mentioned.Message ID: @.***>

jmschrei commented 1 year ago

@SimonBiggs can you please provide an update? I am looking to move this forward. If you are not able to make progress, I can find a replacement.

SimonBiggs commented 1 year ago

Thanks @jmschrei, might be best to find a replacement.

jmschrei commented 1 year ago

Sounds good. Thanks for your work so far.

jmschrei commented 1 year ago

@editorialbot remove @SimonBiggs from reviewers

editorialbot commented 1 year ago

@SimonBiggs removed from the reviewers list!

jmschrei commented 1 year ago

@editorialbot add @anupamajha1 as reviewer

editorialbot commented 1 year ago

@anupamajha1 added to the reviewers list!

gabeerion commented 1 year ago

Second pass is complete for me, with only 2 hopefully small outstanding to-dos detailed in the FrESCO repository. Thanks!

anupamajha1 commented 1 year ago

Review checklist for @anupamajha1

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

anupamajha1 commented 1 year ago

@aspannaus Can you address the following points so that I can complete the corresponding review sections?

  1. Can you add who the target audience of FrESCO is? I can then check off the "A statement of need" check box.

  2. What does MIMIC stand for in the "State of the field" section of the paper?

  3. There was the following warning when decompressing the inference.tar.gz file: "tar: Ignoring unknown extended header keyword `LIBARCHIVE.xattr.com.apple.quarantine'"

  4. Could not run one of the provided models using the commands from the README file. Added details in this issue (https://github.com/DOE-NCI-MOSSAIC/FrESCO/issues/32),

  5. The data preparation and model training sections of the READme are extremely complicated, some complexity can be simplified. For example, the model training section states that to run the model, run this command, "python train_model.py -m ie -args ../configs/model_args.yml." But this command cannot be run as is, the model_args.yml needs to be replaced with one of the actual yml files from the config directory.

  6. Furthermore, the code in the scripts/ directory has very few comments. Please add informative comments to aid understanding of your code flow for a layperson.

aspannaus commented 1 year ago

Hi @anupamajha1,

Thanks for the comments on our repo. Our responses follow:

  1. We added text in the paper at the end of the first paragraph.
  2. This has been defined in the second paragraph.
  3. This has been fixed.
  4. This has been addressed and will be pushed to pypi next week. I'll message here when it is updated on pypi.
  5. The generic model_args.yml file will train a model on the P3B3 dataset. This has been noted in the README.md. We also made some changes to the README to reduce the complexity and improve the organization. If there are more specific ideas that you have, we're happy to iterate.
  6. We have added more comments on the expected use of the scripts in the scripts/ folder and more inline comments.

Thanks for the comments and helpful suggestions.

anupamajha1 commented 1 year ago

@aspannaus Thanks for addressing my comments. I was able to run the code after using pip install -e . command. However, I noticed that FrESCO sometimes produces >1 macro precision and recall and I have started a new issue to let the authors address if this is okay and if not, what is breaking the code (https://github.com/DOE-NCI-MOSSAIC/FrESCO/issues/34)

aspannaus commented 1 year ago

Hi @anupamajha1,

That is strange behavior that we've not seen before. I'll have to do some investigation as to the cause, and will message once it is resolved. Thanks

gabeerion commented 1 year ago

Hi all, my review is complete and all criteria are satisfied. Thanks, and let me know if there is anything else I can do to be helpful!

aspannaus commented 1 year ago

Hi @anupamajha1,

We've looked into the issue you describe, and while we weren't able to reproduce it, we found an issue with the way that the macro scores were being computed within torchmetrics, see: https://github.com/Lightning-AI/torchmetrics/issues/1664 and https://github.com/Lightning-AI/torchmetrics/pull/1821. This doesn't account for the values being > 1, but is concerning nonetheless.

We've modified the code to call the sklearn scoring functions, tested it, and everything is working as expected. The latest version has been updated on pypi as well. Let us know if there's anything else you need from us.

Thanks!

anupamajha1 commented 1 year ago

Thanks for addressing my concerns! I have completed the review checklist and I am fine with the paper in the current state.

aspannaus commented 1 year ago

HI @anupamajha1,

Thank you for the helpful comments and suggestions; we appreciate the feedback to improve the codebase and paper.