search

openjournals / joss-reviews

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

[REVIEW]: OpenCMP: An Open-Source Computational Multiphysics Package #3742

Closed whedon closed 2 years ago

whedon commented 3 years ago

Submitting author: !--author-handle-->@nasserma<!--end-author-handle-- (Nasser Mohieddin Abukhdeir) Repository: https://github.com/uw-comphys/opencmp Branch with paper.md (empty if default branch): publications Version: v1.0.0 Editor: !--editor-->@lucydot<!--end-editor-- Reviewers: @bonh, @WilkAndy Archive: 10.5281/zenodo.6515912

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

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

@bonh & @WilkAndy, 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 @lucydot 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 @bonh

✨ 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 @WilkAndy

✨ 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

nasserma commented 2 years ago

@whedon generate pdf from branch publications

whedon commented 2 years ago 
Attempting PDF compilation from custom branch publications. Reticulating splines etc...
whedon commented 2 years ago

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

 [WARNING] Could not convert image '/tmp/tex2pdf.-d590ef25176202e3/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg': check that rsvg-convert is in path.
  rsvg-convert: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)
Error producing PDF.
! LaTeX Error: Cannot determine size of graphic in /tmp/tex2pdf.-d590ef25176202
e3/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg (no BoundingBox).

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              

l.627 ...b6359d3c5367d353cc654cde8d34a5a41722.svg}

Looks like we failed to compile the PDF
nasserma commented 2 years ago

@whedon generate pdf from branch publications

whedon commented 2 years ago 
Attempting PDF compilation from custom branch publications. Reticulating splines etc...
whedon commented 2 years ago

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

 [WARNING] Could not convert image '/tmp/tex2pdf.-09431f8401f36e88/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg': check that rsvg-convert is in path.
  rsvg-convert: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)
Error producing PDF.
! LaTeX Error: Cannot determine size of graphic in /tmp/tex2pdf.-09431f8401f36e
88/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg (no BoundingBox).

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              

l.627 ...b6359d3c5367d353cc654cde8d34a5a41722.svg}

Looks like we failed to compile the PDF
nasserma commented 2 years ago

I do not understand the issue, it successfully compiles using the preview link,

https://whedon.theoj.org/

using,

https://github.com/uw-comphys/opencmp/

with the branch specified as publications

lucydot commented 2 years ago

Hi @nasserma yes - you are right - it does compile correctly using the preview link you have provided above.

@bonh @WilkAndy I suggest that as a workaround for compiling the paper you use the link https://whedon.theoj.org/ (with https://github.com/uw-comphys/opencmp/ as the repo and the branch specified as publications).

I'll look into why whedon command @whedon generate pdf from branch publications isn't working.

lucydot commented 2 years ago

@nasserma @bonh @WilkAndy please note that I am setting my out of office for the next two weeks over Christmas. I will check in once a week, but my responses might be slower than usual for the next fortnight.

lucydot commented 2 years ago

/ooo December 20 until January 3

lucydot commented 2 years ago

@openjournals/devs - pinging you here as we are having problems compiling the paper with the command @whedon generate pdf from branch publications. Compiling using the link https://whedon.theoj.org/ with https://github.com/uw-comphys/opencmp/ as the repo and publications as the branch is working.

lucydot commented 2 years ago

@whedon generate pdf from branch publications

whedon commented 2 years ago 
Attempting PDF compilation from custom branch publications. Reticulating splines etc...
whedon commented 2 years ago

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

 [WARNING] Could not convert image '/tmp/tex2pdf.-fc42df7ef731c975/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg': check that rsvg-convert is in path.
  rsvg-convert: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)
Error producing PDF.
! LaTeX Error: Cannot determine size of graphic in /tmp/tex2pdf.-fc42df7ef731c9
75/6bfdb6359d3c5367d353cc654cde8d34a5a41722.svg (no BoundingBox).

See the LaTeX manual or LaTeX Companion for explanation.
Type  H <return>  for immediate help.
 ...                                              

l.627 ...b6359d3c5367d353cc654cde8d34a5a41722.svg}

Looks like we failed to compile the PDF
bonh commented 2 years ago

On documentation: uw-comphys/opencmp#18

lucydot commented 2 years ago

Hello @nasserma @Alex-Vasile and our reviewers @bonh @WilkAndy -

I'm aware this review has been going for > 3months so thought I'd do a quick summary of where we are at now, and try to establish a timeframe for the review.

Looking at the issues that are open:

@bonh @WilkAndy, with the above in mind, could I ask you to update your tick-boxes (if appropriate) so I can get a feel for where we are at in the review process?

bonh commented 2 years ago

Hi,

lucydot commented 2 years ago

Hi @bonh @WilkAndy, have either of you been able to succesfully run the tests after installation? As highlighted above, this is the current sticking point. The relevant issue thread (uw-comphys/opencmp#19) has been updated and apt-get has been suggested rather than using conda for install.

nasserma commented 2 years ago

Hi @lucydot , @bonh was able to run all of our tests, please see the thread here,

https://github.com/uw-comphys/opencmp/issues/19

there was an issue with the pytest module's parsing of the --runslow option, which is corrected in our documentation. From my understanding we have addressed all of the reviewers' comments and, with our most recent version, now have two tutorials which demonstrate 3D simulations both using the diffuse-interface method and flow past an immersed cylinder.

The last open issue is,

https://github.com/uw-comphys/opencmp/issues/8

but we have addressed all of the issues, apart from adding simulation results images to the paper. We can do this, but since we provide links to the examples/tutorials and images/movies are available through our website, we think that is reasonable unless there is a strong objection.

lucydot commented 2 years ago

Hello @nasserma - thank you for the update. @bonh @WilkAndy if you are able to give us updates on your review in the next fortnight or so that would very useful. I'm aware this review has been taking some time, hopefully now we are at a stage where the functionality and central claims of the software can be assessed. If you have any questions or are unavailable to review over the immediate time period, please let me know.

WilkAndy commented 2 years ago

I'm kind of pulling my hair out over here in Australia!! I'm pretty sure i installed and tested the software, but have since got a new computer and am having lots of trouble installing ngsolve. It's a real beast with the conda version not working, the mac version not working properly with opencmp, and properly testing opencmp being problematic (for me) on windows.

I'm disappointed that the authors don't want to enhance their paper in various ways i suggested above, because i feel it'd encourage readers to try opencmp, but that's no reason for rejecting the manuscript. So, i think i "just" have to install ngsolve.

bonh commented 2 years ago

I get the feeling that you, @nasserma, and your coauthors find me rather annoying to be honest. If so, I am really sorry, but I think it is my duty to be thorough and a little pushy. Sadly, you seldom engage in a discussion with me.

I would like to reopen uw-comphys/opencmp/issues/18 as I am not done with the documentation yet. Same for uw-comphys/opencmp/issues/6 as I was stuck with testing and did not look at the 3D simulations yet. If you close the issues I loose track, sorry about that.

I am continue now with trying out a few simulations but I am convinced by now that this will be publishable.

@WilkAndy I ended up putting it into a docker container (Ubuntu and then a lot of apt get and pip from inside the container). If you want I can try to share it with you (it is not nice...).

nasserma commented 2 years ago

I'm kind of pulling my hair out over here in Australia!! I'm pretty sure i installed and tested the software, but have since got a new computer and am having lots of trouble installing ngsolve. It's a real beast with the conda version not working, the mac version not working properly with opencmp, and properly testing opencmp being problematic (for me) on windows.

I'm disappointed that the authors don't want to enhance their paper in various ways i suggested above, because i feel it'd encourage readers to try opencmp, but that's no reason for rejecting the manuscript. So, i think i "just" have to install ngsolve.

Sorry that you are having trouble with the installation using Anaconda and MacOS, we will definitely work with the NGSolve developers to try and find out what issues exist and how to fix them. Similarly to you having issues testing on these platforms, we have issues testing on other platforms since we predominantly use some variation of Linux for desktop and high-performance computing.

Regarding your second comment, if you reread my original response from a few days ago I indicated that we would make this change if there were strong objections. This is something we can complete over the weekend and will push a new version of the manuscript for you to review on Monday.

bonh commented 2 years ago

I started to carefully read the paper. Comments are in https://github.com/uw-comphys/opencmp/issues/22.

nasserma commented 2 years ago

I get the feeling that you, @nasserma, and your coauthors find me rather annoying to be honest. If so, I am really sorry, but I think it is my duty to be thorough and a little pushy. Sadly, you seldom engage in a discussion with me.

I would like to reopen uw-comphys/opencmp/issues/18 as I am not done with the documentation yet. Same for uw-comphys/opencmp/issues/6 as I was stuck with testing and did not look at the 3D simulations yet. If you close the issues I loose track, sorry about that.

I am continue now with trying out a few simulations but I am convinced by now that this will be publishable.

I am sorry if you get the feeling that I am annoyed with you or am not engaging in conversation. You have to understand, this is a very different peer review process from what I am accustomed to. I am doing my best to adapt to it, but I also must consult with my co-authors before responding, making changes to the code, and revising the manuscript. This takes time, as I am sure you understand.

Regarding the two issues with the code that you found, we are all very sorry for not catching those and will correct this in the future by implement automated testing of the examples, in addition to the unit tests/pytests. These were introduced by some refactoring and it was my responsibility to test the examples. We will add this to our to-do list for the weekend!

Please check back on Monday, and make sure to use the publications branch, which we'll merge into the main branch once all of these issues are resolved.

lucydot commented 2 years ago

@bonh @WilkAndy thank-you for continuing to donate your time to the review process, in spite of the hair-pulling and feeling that your comments are not being responded to fully. I can see that there is continued progress being made with the paper (opencmp/issues22). Install has been very challenging, there is now an issue (opencmp/issues23) offering further support.

Even though we are at the 5-month mark, which is beyond the typical timeframe for JOSS reviews, my thoughts are that we should try to push forwards with this, as long as there is continued progress with improving the code and documenation. I'd like to know your thoughts on this?

@nasserma the point you raise about this being a different type review process is very true! This style of conversation - the back and forth - and the informality of it - is very different from the process in traditional journals. Personally this is what I think makes JOSS a very effective journal, as it (hopefully) allows for more open conversation, and recognises that software development is a iterative process. I've found that reviews in which authors are communicative - especially when they come up against barriers and are struggling to find solutions to the points the reviewers make - are the most successful.

nasserma commented 2 years ago

@lucydot @WilkAndy @bonh I have pushed a revised manuscript with changes meant to address all of the issues and comments from the open issues (#8, 22, 23). I am having trouble with the layout of images, but I want to get an otherwise complete revision out while I sort out using the JOSS docker image to compile locally.

Please let me know what you all think!

nasserma commented 2 years ago

@whedon generate pdf from branch publications

whedon commented 2 years ago 
Attempting PDF compilation from custom branch publications. Reticulating splines etc...
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:

bonh commented 2 years ago

@lucydot Sure, lets continue the review!

@nasserma What about that one from above:

I would like to reopen https://github.com/uw-comphys/opencmp/issues/18 as I am not done with the documentation yet. Same for https://github.com/uw-comphys/opencmp/issues/6 as I was stuck with testing and did not look at the 3D simulations yet. If you close the issues I loose track, sorry about that.

Perhaps you could reopen them?

bonh commented 2 years ago

I read through the paper again and added some new discussion points in https://github.com/uw-comphys/opencmp/issues/22. And there are some old points which have not been addressed yet.

nasserma commented 2 years ago

I reopened them.

I would like to reopen uw-comphys/opencmp#18 as I am not done with the documentation yet. Same for uw-comphys/opencmp#6 as I was stuck with testing and did not look at the 3D simulations yet. If you close the issues I loose track, sorry about that.

Perhaps you could reopen them?

nasserma commented 2 years ago

I responded to all of your new comments, however, you indicated that a previous comment was not responded to but I could not determine which one you were referring to.

I read through the paper again and added some new discussion points in uw-comphys/opencmp#22. And there are some old points which have not been addressed yet.

editorialbot commented 2 years ago

My name is now @editorialbot

nasserma commented 2 years ago

@editorialbot generate pdf from branch publications

editorialbot commented 2 years ago

I'm sorry human, I don't understand that. You can see what commands I support by typing:

@editorialbot commands

danielskatz commented 2 years ago

@editorialbot set publications as branch

editorialbot commented 2 years ago

Done! branch is now publications

danielskatz commented 2 years ago

@editorialbot generate pdf

editorialbot commented 2 years ago

:warning: An error happened when generating the pdf.

danielskatz commented 2 years ago

@xuanxu - Can you please look at this error

/home/runner/work/_actions/xuanxu/paper-action/main/vendor/bundle/ruby/3.1.0/gems/theoj-1.2.1/lib/theoj/paper.rb:155:in `failure': Couldn't check the bibtex because branch name is incorrect: publications (Theoj::Error)

nasserma commented 2 years ago

@xuanxu - Can you please look at this error

/home/runner/work/_actions/xuanxu/paper-action/main/vendor/bundle/ruby/3.1.0/gems/theoj-1.2.1/lib/theoj/paper.rb:155:in `failure': Couldn't check the bibtex because branch name is incorrect: publications (Theoj::Error)

@lucydot @WilkAndy @bonh Note that the link above does not actually point towards the updated manuscript PDF. Until this issue is resolved, I think it might have to do with some changes to the compilation process via Github, could you all please compile and view the paper through the preview service,

https://whedon.theoj.org/ Paper repository address: https://github.com/uw-comphys/opencmp Custom Branch: publications

I have confirmed that the paper compiles and successfully generates a preview PDF again just now.

xuanxu commented 2 years ago

Error found (ambiguity between branch name and folder name) and hopefully fixed, let's try again.

xuanxu commented 2 years ago

@editorialbot generate pdf

editorialbot 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:

xuanxu commented 2 years ago

@editorialbot check references

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

OK DOIs

- 10.1002/nme.5628 is OK
- 10.1002/nme.2579 is OK
- 10.1002/cjce.24320 is OK
- 10.1146/annurev.fluid.37.061903.175743 is OK
- 10.11588/ans.2015.100.20553 is OK
- 10.1016/j.softx.2020.100430 is OK
- 10.1137/0732037 is OK

MISSING DOIs

- 10.1504/ijcse.2012.048245 may be a valid DOI for title: Benchmark computations of 3D laminar flow around a cylinder with CFX, OpenFOAM and FeatFlow
- 10.1016/b978-012387582-2/50038-1 may be a valid DOI for title: ParaView: An End-User Tool for Large Data Visualization
- 10.1090/s0025-5718-03-01552-7 may be a valid DOI for title: The Local Discontinuous Galerkin Method for the Oseen Equations
- 10.1109/iccse.2015.7250335 may be a valid DOI for title: Recent Algorithms on Automatic Hexahedral Mesh Generation

INVALID DOIs

- None
nasserma commented 2 years ago

@lucydot @WilkAndy @bonh I have confirmed that the latest PDF is the correct/updated version of the manuscript. Sorry for not catching this earlier, but the preview PDF built correctly and I did not check the Github version thinking that it would be exactly the same.

Since the revised manuscript is significantly different than before, I am hoping that both reviewers can revisit their comments and identify which ones are addressed satisfactorily and which ones are not. Thanks.

lucydot commented 2 years ago

@editorialbot check references