search

openjournals / joss-reviews

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

[REVIEW]: BioGears: A C++ library for whole body physiology simulations #2645

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @ajbaird (Austin Baird) Repository: https://github.com/BioGearsEngine/core Version: v7.3.2 Editor: @meg-simula Reviewer: @fcooper8472, @MiroK Archive: 10.5281/zenodo.4304604

: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/5ecd8c3138fe9e272179549c49905635"><img src="https://joss.theoj.org/papers/5ecd8c3138fe9e272179549c49905635/status.svg"></a>
Markdown: [![status](https://joss.theoj.org/papers/5ecd8c3138fe9e272179549c49905635/status.svg)](https://joss.theoj.org/papers/5ecd8c3138fe9e272179549c49905635)

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

@fcooper8472 & @MiroK, 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 @meg-simula 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 @fcooper8472

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @MiroK

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 4 years ago

Hello human, I'm @whedon, a robot that can help you with some common editorial tasks. @fcooper8472, @MiroK 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 4 years ago

PDF failed to compile for issue #2645 with the following error:

Can't find any papers to compile :-(

meg-simula commented 4 years ago

@whedon generate pdf from branch f/abard-JOSSPaper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch f/abard-JOSSPaper. Reticulating splines etc...
meg-simula commented 4 years ago

@whedon check references from branch f/abard-JOSSPaper

whedon commented 4 years ago 
Attempting to check references... from custom branch f/abard-JOSSPaper
whedon commented 4 years ago

PDF failed to compile for issue #2645 with the following error:

sh: 1: cd: can't cd to tmp/2645 /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in block in find': No such file or directory - tmp/2645 (Errno::ENOENT) from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:incollect!' from /app/vendor/ruby-2.4.4/lib/ruby/2.4.0/find.rb:43:in find' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/lib/whedon/processor.rb:61:infind_paper_paths' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/bin/whedon:50:in prepare' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.4.0/gems/thor-0.20.3/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.4.0/bundler/gems/whedon-132474d2711b/bin/whedon:119:in<top (required)>' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in load' from /app/vendor/bundle/ruby/2.4.0/bin/whedon:23:in

'

whedon commented 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- None

MISSING DOIs

- 10.1089/end.2016.0613 may be a valid DOI for title: The CREST simulation development process: training the next generation
- 10.1002/psp4.12371 may be a valid DOI for title: Open source pharmacokinetic/pharmacodynamic framework: tutorial on the BioGears Engine
- 10.1117/12.2252510 may be a valid DOI for title: Physiology informed virtual surgical planning: a case study with a virtual airway surgical planner and BioGears
- 10.3389/fphys.2019.01321 may be a valid DOI for title: A Whole-Body Mathematical Model of Sepsis Progression and Treatment Designed in the BioGears Physiology Engine
- 10.1109/embc.2019.8857686 may be a valid DOI for title: A Full-Body Model of Burn Pathophysiology and Treatment Using the BioGears Engine

INVALID DOIs

- None
meg-simula commented 4 years ago

@ajbaird Could you look at the paper compilation issue reported by whedon above at your earliest convenience?

ajbaird commented 4 years ago

sorry just noticed this, I can update DOIs today, Thanks!

ajbaird commented 4 years ago

Hi @meg-simula just pushed an update for the missing DOIs, not totally sure about the compilation error though? Seems to be not able to find the temp/ folder and the cd command is failing? Compiled ok before, maybe it needs to update with the new issue number once it got moved? I'm really not sure, sorry!

meg-simula commented 4 years ago

@whedon generate pdf from branch f/abard-JOSSPaper

whedon commented 4 years ago 
Attempting PDF compilation from custom branch f/abard-JOSSPaper. Reticulating splines etc...
whedon commented 4 years ago

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

meg-simula commented 4 years ago

@whedon check references from branch f/abard-JOSSPaper

whedon commented 4 years ago 
Attempting to check references... from custom branch f/abard-JOSSPaper
whedon commented 4 years ago 
Reference check summary (note 'MISSING' DOIs are suggestions that need verification):

OK DOIs

- 10.1089/end.2016.0613 is OK
- 10.1016/0022-2828(90)91460-o is OK
- 10.1007/s11517-008-0359-2 is OK
- 10.1002/psp4.12371 is OK
- 10.1117/12.2252510 is OK
- 10.3389/fphys.2019.01321 is OK
- 10.1109/EMBC.2019.8857686 is OK

MISSING DOIs

- None

INVALID DOIs

- None
meg-simula commented 4 years ago

@ajbaird Ok, thanks! Compiles nicely now.

ajbaird commented 4 years ago

@meg-simula awesome, sorry about that issue!

arfon commented 3 years ago

:wave: @fcooper8472, @MiroK, @meg-simula - how are things going here? I see that @fcooper8472 & @MiroK haven't made much progress with your reviews yet?

fcooper8472 commented 3 years ago

@arfon you're absolutely right - my apologies! I'll get cracking on it this afternoon.

MiroK commented 3 years ago

Sorry for the delay. I'll get to it next week

fcooper8472 commented 3 years ago

@arfon I'm having problems configuring with CMake on Ubuntu 20.04. I realise there aren't any specific instructions for 20.04, but the problematic dependency is identical to that on 18.04, so perhaps I'm missing something:

I'm following these instructions:

installing xsd

wget https://www.codesynthesis.com/download/xsd/4.0/linux-gnu/x86_64/xsd_4.0.0-1_amd64.deb
sudo dpkg -i xsd_4.0.0-1_amd64.deb

But get, when running CMake:

CMake Warning at cmake/cmake-common_logic.cmake:42 (message):
  The following packages CodeSynthesis were not found.For native compilations
  setting CMAKE_PREFIX_PATH can solve this problemFor cross complilation try
  expanding your CMAKE_FIND_ROOT_PATH
Call Stack (most recent call first):
  projects/biogears/CMakeLists.txt:34 (verify_package)

I see that ultimately the script that is responsible for finding XSD is

core/cmake/common/FindCodeSynthesis.cmake

but I can't see a mechanism to, for instance, explicitly identify paths to search in for XSD. Any ideas?

arfon commented 3 years ago

@arfon I'm having problems configuring with CMake on Ubuntu 20.04. I realise there aren't any specific instructions for 20.04, but the problematic dependency is identical to that on 18.04, so perhaps I'm missing something:

@ajbaird - I think this question is actually for you :-)

fcooper8472 commented 3 years ago

@arfon I'm having problems configuring with CMake on Ubuntu 20.04. I realise there aren't any specific instructions for 20.04, but the problematic dependency is identical to that on 18.04, so perhaps I'm missing something:

@ajbaird - I think this question is actually for you :-)

Aha yes, my bad!

ajbaird commented 3 years ago

Hi @fcooper8472 @arfon ! So I think there are two solutions to the issue you are having, you can opt to use the xsd version provided in the apt-get package manager, I think its called xsdcxx (we haven't tested using this but I think it'll probably work, the brew version works on macOS).

I feel like with our testing, we didn't need to configure cmake prefix paths to point to xsd on linux (when using the .deb package), but you could get the .bz2 file, unzip it and then add a line to the configuration step to set CMAKE_PREFIX_PATH=

fcooper8472 commented 3 years ago

Thanks @ajbaird, got it working by unzipping the bz2 package and specifying CMAKE_PREFIX_PATH. I then get a warning:

-- Configuring Biogears
-- Could NOT find Boost: Found unsuitable version "1.71.0", but required is at least "1.73.0" (found /usr/include, )

but can't see how to specify the boost location AND the XSD location using CMAKE_PREFIX_PATH.

Without boost, though, it seems to have configured and compiled fine.

ajbaird commented 3 years ago

Ok, yeah there should just be a check for the version of boost and we rely on a few optional parameters from 1.73, so as long as it compiled ok, I believe you are good to go! We are updating the wiki for your 20.04 build to mention unzipping the bz2 package instead of relying on the .deb package! Thanks for the input.

fcooper8472 commented 3 years ago

Apologies for the delay in completing my review. I have only one checkbox unticked:

Some general comments on the software/repository:

Some general comments on the paper:

ajbaird commented 3 years ago

Hi @fcooper8472 we use build bot for our CI processes, we have developed it in house using those python libraries to build our cross platform tests, we don't' give access to others to view that splash screen but the badges are generated with buildbot, then uploaded to AWS so the badges are updated nightly. I can def change the general comments this week!

Getting access to our IRC channel is the best way to contribute and we do have a very brief wiki article but I updating the contributing link is something that has been on our radar and needs updating.

ajbaird commented 3 years ago

oh just saw the top comment @fcooper8472, I can update some information there, should I do that now or after this process has concluded, then re-submit?

fcooper8472 commented 3 years ago

@ajbaird I think you can just update the paper and then get the bot to rebuild the paper as a new pdf. Definitely don't need to re-submit I don't think.

ajbaird commented 3 years ago

@fcooper8472 ok great! I'll update now, might be able to wrap it up today. I'll post here when done.

ajbaird commented 3 years ago

@whedon generate pdf from branch f/abard-JOSSPaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch f/abard-JOSSPaper. Reticulating splines etc...
whedon commented 3 years ago

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

ajbaird commented 3 years ago

ok @fcooper8472 just made some changes, I included the bulk of the text changes to address the unchecked box under the summery title. If you think it should have its own header, let me know, I thought it sort of fit there.

ajbaird commented 3 years ago

@whedon generate pdf from branch f/abard-JOSSPaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch f/abard-JOSSPaper. Reticulating splines etc...
whedon commented 3 years ago

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

fcooper8472 commented 3 years ago

@ajbaird That looks great, I'm happy to check the box now.

I'm finished with my review now; it all looks good from my perspective.

meg-simula commented 3 years ago

Thanks @fcooper8472 for the completed review and @ajbaird for the rapid handling of feedback.

meg-simula commented 3 years ago

@MiroK How are you progressing with this review? Do let me know if there is anything I can contribute with at this stage.

ajbaird commented 3 years ago

let me know if you need anything from me!

MiroK commented 3 years ago

@ajbaird I apologize for late response. From my point of view all looks/works nicely; I was able to install and run the code without issues on my Ubuntu 18.04. The only comments I have are regarding the article proof:

ajbaird commented 3 years ago

Hi @MiroK ! Thanks for the feedback, glad you were able to get it up and running! I'll address these two items today and build a new pdf.

ajbaird commented 3 years ago

@whedon generate pdf from branch f/abard-JOSSPaper

whedon commented 3 years ago 
Attempting PDF compilation from custom branch f/abard-JOSSPaper. Reticulating splines etc...
whedon commented 3 years ago

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

ajbaird commented 3 years ago

Hi @MiroK got those edits in, feel free to download the proof above!

Thanks for the edits!