openjournals / joss-reviews

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

[REVIEW]: BVPy: A Python package based on FEniCS and Gmsh for solving boundary value problems in cell and tissue mechanics. #2831

Closed whedon closed 3 years ago

whedon commented 4 years ago

Submitting author: @oalii (Olivier Ali) Repository: https://gitlab.com/oali/bvpy Version: v1.0.1 Editor: @Kevin-Mattheus-Moerman Reviewer: @IgorBaratta, @chennachaos, @finsberg Archive: 10.5281/zenodo.4590758

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

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

@IgorBaratta & @chennachaos & @finsberg, 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 @Kevin-Mattheus-Moerman 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 @IgorBaratta

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @chennachaos

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @finsberg

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

chennachaos commented 3 years ago

Hi @oalii. Thank you for the updates!

@Kevin-Mattheus-Moerman, I have gone through the updates made by the authors. The present library does not make any substantial contribution to computing the solution of PDEs that can not already be solved by FEniCS. The example on Hyperelasticity seems superfluous. This example considers only surfaces in 3D. The only positive aspect of this library is the customised post-processing functionality but an FEA library is not just post-processing and nice documentation, it is the core and it already exists in FEniCS. Note here that my previous comment is based on the fact that the authors use an existing core FE library (FEniCS) and that bvpy does not solve nonlinear PDEs that model complex multi-physical interactions; only some standard well-established PDEs are considered. Note also that the authors are not proposing any new formulations or techniques for solving these standard PDEs. The overall contribution is minimal, incremental on top of FEniCS, that too for problems that FEniCS itself can solve.

I still maintain my decision. I do not recommend this library for publication.

oalii commented 3 years ago

@whedon generate pdf from branch joss-review

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-review. 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:

oalii commented 3 years ago

Hi @Kevin-Mattheus-Moerman and all reviewers,

@chennachaos: thanks for your latest comment.

We acknowledge that, given his scientific expertise, @chennachaos seeks novelty in algorithms and do not consider BVPy as bringing any useful new features to expert users. We would like to reply in two points:

Finally, to answer @chennachaos concerns and make our contribution more precise, we amended the “summary”, “statement of need” and “state of the field” sections of the paper. We even propose a modified title for the paper (BVPy: A FEniCS-based Python package to ease the expression and study of boundary value problems in Biology.) if @Kevin-Mattheus-Moerman thinks it better describes our work and benefits the audience as well as the journal.

oalii commented 3 years ago

@whedon generate pdf from branch joss-review

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-review. 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:

oalii commented 3 years ago

Hi @Kevin-Mattheus-Moerman and @IgorBaratta ,

Any update on your side ?

Kevin-Mattheus-Moerman commented 3 years ago

Thanks for the reminder @oalii and apologies for the delay. Yes I'll pick things up now.

Kevin-Mattheus-Moerman commented 3 years ago

@IgorBaratta thanks for your help here. Your last unchecked box relates to "Performance". Can you comment on what is holding this back? Or have the recent improvements address your issues? Thanks

Kevin-Mattheus-Moerman commented 3 years ago

@chennachaos thanks for your detailed review efforts!! I understand your comments about whether BVPy is a "substantial contribution". Note that JOSS does not require novelty in terms of functionality. For instance complete re-implementations of functionality in different languages may be acceptable provided this improves/enables/expands use and/or applications. The former includes expanding the use or making it more user friendly for a target or non-expert community. I think @oalii has provided some comments as to the value of BVPy in that respect.

Can you provide feedback to @oalii's comments to the issues you raised (also in light of my comments here)?

Let me know if you have any questions on the scope/scholarly effort/functionality requirements for JOSS (https://joss.readthedocs.io/en/latest/submitting.html).

Thanks!

IgorBaratta commented 3 years ago

@IgorBaratta thanks for your help here. Your last unchecked box relates to "Performance". Can you comment on what is holding this back? Or have the recent improvements address your issues? Thanks

There are no "Performance claims" so I'm checking off this item, as per the reviewer guidelines. The software does not perform parallel computing, which is one of the principles of FEniCS ( in which the software is based). However, @oalii has provided some comments in that respect: https://github.com/openjournals/joss-reviews/issues/2831#issuecomment-762945323.

Since "Performance" is not a limiting factor for acceptance, I can recommend BVPy for publication.

Kevin-Mattheus-Moerman commented 3 years ago

@IgorBaratta thanks for your comments and your help here!

Kevin-Mattheus-Moerman commented 3 years ago

@chennachaos I hope are well. I was wondering if you could comment on my reply :point_up:, and pick up this review. It is okay if you need more time also, just let us know. Thanks again for your help.

chennachaos commented 3 years ago

Hi @Kevin-Mattheus-Moerman The main problem is not about novelty but the amount of contribution. If you think that the contribution is sufficient for publication in JOSS, then I do not have any further comments. The other parts of the review, that is documentation and installation, are satisfactory.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon check repository

whedon commented 3 years ago
Software report (experimental):

github.com/AlDanial/cloc v 1.88  T=0.53 s (159.5 files/s, 26333.0 lines/s)
-------------------------------------------------------------------------------
Language                     files          blank        comment           code
-------------------------------------------------------------------------------
Python                          52           1587           3096           3458
Jupyter Notebook                 9              0           3472            443
DOS Batch                        1             29              3            212
Markdown                         2            106              0            198
reStructuredText                 9            206            170            174
YAML                             3             16             10            173
make                             1             29              8            143
JSON                             3              0              0            130
TeX                              1              8              0            118
Dockerfile                       1             14              8             39
CSS                              1              0              0              9
Bourne Shell                     1              2              0              4
-------------------------------------------------------------------------------
SUM:                            84           1997           6767           5101
-------------------------------------------------------------------------------

Statistical information for the repository '85485b2706e7639e1a0c9434' was
gathered on 2021/02/26.
The following historical commit information, by author, was found:

Author                     Commits    Insertions      Deletions    % of changes
Florian                        229         15927          11002           66.06
GACON Florian                    4           228              2            0.56
Olivier Ali                    138         11122           2484           33.38

Below are the number of rows from each author that have survived and are still
intact in the current revision:

Author                     Rows      Stability          Age       % in comments
Florian                    4524           28.4          1.0               16.53
Olivier Ali                3617           32.5          1.0                9.43
Kevin-Mattheus-Moerman commented 3 years ago

@whedon generate pdf from branch joss-review

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-review. 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:

Kevin-Mattheus-Moerman commented 3 years ago

@chennachaos thanks for your continued help here. I've gone over all the review comments and author responses again, and have reviewed the paper and magnitude of this contribution again too (in terms of lines of code but also functionality offered). I understand your concerns in relation to the "amount of contribution". Given the positive comments by two reviewers and the clarifications the author provided about the scope and users this submission is targeted at, I feel that this submission is substantial enough in terms of the functionality it offers to be in scope for JOSS.

I would be grateful if you could tick as many boxes as you see fit :point_up:, for instance, if you are in agreement, those relating to installation, documentation, and the paper. If, after doing this, no other pressing issues remain then I will likely recommend this work for publication. Note however that if you like, you could leave the Substantial scholarly effort (which seems most closely related to your concerns) box unticked.

Kevin-Mattheus-Moerman commented 3 years ago

@chennachaos can you reply to my comment :point_up:, do you think you can tick as many boxes as you feel possible? Thanks!

chennachaos commented 3 years ago

Hello @Kevin-Mattheus-Moerman. Apologies for the delay. Been occupied with a lot of teaching and admin activities. I have ticked the boxes now. Except for my concern regarding the amount of work, everything else is fine with this library. Thank you for the opportunity once again.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon generate pdf

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:

Kevin-Mattheus-Moerman commented 3 years ago

@whedon check references

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

OK DOIs

- 10.1002/nme.2579 is OK
- 10.1016/j.tplants.2010.04.002 is OK
- 10.1016/j.physa.2017.02.014 is OK
- 10.1126/science.251.4994.650 is OK
- 10.1016/j.cub.2020.07.076 is OK
- 10.5281/zenodo.4090832 is OK

MISSING DOIs

- None

INVALID DOIs

- None
Kevin-Mattheus-Moerman commented 3 years ago

@oalii congrats we will now process the last steps to help accept this work in JOSS.

Below are some final suggested changes on the paper:

After you've processed the above:

Kevin-Mattheus-Moerman commented 3 years ago

Thanks for your help @chennachaos!

oalii commented 3 years ago

@whedon generate pdf from branch joss-review

whedon commented 3 years ago
Attempting PDF compilation from custom branch joss-review. 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:

oalii commented 3 years ago

@Kevin-Mattheus-Moerman,

That's a great news, thanks! we are very happy and proud :) Concerning the last remarks:

We are currently looking into the Zenodo archive procedure and let you know as soon as it is done. We expect to complete it by tomorrow at the latest.

Thanks again for your help and the solid work of all reviewers !

oalii commented 3 years ago

@Kevin-Mattheus-Moerman,

Sorry to bother, but I have a quick question concerning the Zenodo archive: The procedure you pointed to is related to GitHub repositories... And we are using a Gitlab one, which apparently does not support an automated procedure to create an archive every time a new release is generated...

My question: would a Zenodo archive simply based on a "zip" of the library (V 1.0.0.) be sufficient ? If not, do you know how we could generate automatic archive from Gitlab ? If not, should we mirror our repo on Github and generate the archive from there ?

Thx for your insight.

Kevin-Mattheus-Moerman commented 3 years ago

No need to mirror. You can just upload the Gitlab content to Zenodo (so no need to set up the automated github approach). Just make sure to check all the meta data (there may be fields to enter to say it is software also).

Here is another accepted submission which used Gitlab I think they did the same: https://joss.theoj.org/papers/10.21105/joss.02165

oalii commented 3 years ago

Thanks !

oalii commented 3 years ago

@Kevin-Mattheus-Moerman,

A few remarks:

Is it ok for you ? Let me know, thanks,

Kevin-Mattheus-Moerman commented 3 years ago

@whedon set 10.5281/zenodo.4590758 as archive

whedon commented 3 years ago

OK. 10.5281/zenodo.4590758 is the archive.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon set v1.0.1 as version

whedon commented 3 years ago

OK. v1.0.1 is the version.

Kevin-Mattheus-Moerman commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

/app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:in parse': (e0eb212ceaecb4955ceaf8f0/paper/paper.md): could not find expected ':' while scanning a simple key at line 22 column 1 (Psych::SyntaxError) from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:inparse_stream' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:390:in parse' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:277:inload' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:578:in block in load_file' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:inopen' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:in load_file' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:127:inload_yaml' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:87:in initialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:innew' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:in set_paper' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:58:inprepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in <top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `

'

Kevin-Mattheus-Moerman commented 3 years ago

@oalii Can you check this error? Did you move/break the paper? Can you update the paper here?

As you can see :point_up: I have updated the version tag and set the archive link.

oalii commented 3 years ago

@whedon generate pdf

whedon commented 3 years ago

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

/app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:in parse': (b6becd530a0609deaf51dd85/paper/paper.md): could not find expected ':' while scanning a simple key at line 22 column 1 (Psych::SyntaxError) from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:inparse_stream' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:390:in parse' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:277:inload' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:578:in block in load_file' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:inopen' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:in load_file' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:127:inload_yaml' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:87:in initialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:innew' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:in set_paper' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:58:inprepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in <top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `

'

oalii commented 3 years ago

@whedon generate pdf from branch master

whedon commented 3 years ago
Attempting PDF compilation from custom branch master. Reticulating splines etc...
whedon commented 3 years ago

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

/app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:in parse': (df4efce0c78135100ae1eabc/paper/paper.md): could not find expected ':' while scanning a simple key at line 22 column 1 (Psych::SyntaxError) from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:456:inparse_stream' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:390:in parse' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:277:inload' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:578:in block in load_file' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:inopen' from /app/vendor/ruby-2.6.6/lib/ruby/2.6.0/psych.rb:577:in load_file' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:127:inload_yaml' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon.rb:87:in initialize' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:innew' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/lib/whedon/processor.rb:38:in set_paper' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:58:inprepare' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/command.rb:27:in run' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/invocation.rb:126:ininvoke_command' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor.rb:387:in dispatch' from /app/vendor/bundle/ruby/2.6.0/gems/thor-0.20.3/lib/thor/base.rb:466:instart' from /app/vendor/bundle/ruby/2.6.0/bundler/gems/whedon-92346a0773a4/bin/whedon:131:in <top (required)>' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:inload' from /app/vendor/bundle/ruby/2.6.0/bin/whedon:23:in `

'