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]: FEST-3D: Finite-volume Explicit STructured 3-Dimensional solver #1555

Closed whedon closed 4 years ago

whedon commented 5 years ago

Submitting author: @jayten (Jatinder Pal Singh Sandhu) Repository: https://github.com/FEST3D/FEST-3D Version: v1.1 Editor: @labarba Reviewers: @hariradh, @rouson Archive: 10.5281/zenodo.3660408

Status

status

Status badge code:

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

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

@hariradh, @rouson 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 @labarba know.

✨ Please try and complete your review in the next two weeks ✨

Review checklist for @hariradh

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

Review checklist for @rouson

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 5 years ago

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

: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
whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

labarba commented 5 years ago

@whedon add @rouson as reviewer

whedon commented 5 years ago

OK, @rouson is now a reviewer

labarba commented 5 years ago

👋 @hariradh, @rouson — This is where the action happens. You each have a review checklist above to work through. As you do, feel free to post your reviewer comments to the author in this thread. You may also open issues in the software repository as needed, and post a link here. Ask me anything!

Reviewer Guidelines on our dedicated documentation site 🚀

hariradh commented 5 years ago

I have trouble compiling the code based on the instructions. The cmake step hangs.

$ FC=mpif90 cmake ..
-- The Fortran compiler identification is GNU 9.1.0
-- Checking whether Fortran compiler has -isysroot
-- Checking whether Fortran compiler has -isysroot - yes
-- Checking whether Fortran compiler supports OSX deployment target flag
-- Checking whether Fortran compiler supports OSX deployment target flag - yes
-- Check for working Fortran compiler: /usr/local/bin/mpif90
-- Check for working Fortran compiler: /usr/local/bin/mpif90  -- works
-- Detecting Fortran compiler ABI info
-- Detecting Fortran compiler ABI info - done
-- Checking whether /usr/local/bin/mpif90 supports Fortran 90
-- Checking whether /usr/local/bin/mpif90 supports Fortran 90 -- yes
-- CMAKE_BUILD_TYPE not given, defaulting to RELEASE

I am testing the code using:

Cmake, gcc, and MPICH2 were installed using homebrew.

jayten commented 5 years ago

The compilation problem on MacOSX has been sorted out. The following piece of code in the CMake module "SetFortranFlag.cmake" was creating a problem. 

# Don't add underscores in symbols for C-compatibility
SET_COMPILE_FLAG(CMAKE_Fortran_FLAGS "${CMAKE_Fortran_FLAGS}"
                 Fortran "-fno-underscoring")

So, the issue was corrected on Fixed the Compatibility issue of MacOSX for CMake script branch and merged with master.

The latest state of the code on master branch has been test on OSX version 10.13.4 using following:

Compilation was done using $ FC=mpif90 cmake .. and then $ make -j 4

labarba commented 5 years ago

👋 @hariradh, @rouson — how are you getting on with this review? I see the author has made some fixes, but no more comments in the last 3 weeks. Can we have a status update and an estimate of the time needed to complete the reviews?

hariradh commented 5 years ago

@labarba @jayten

I apologize for the delay but I am having trouble testing the functionality of the code. The code fails when running the automaton.py script in the CheckInput function. I was trying to figure out the cause before sending my comments to the authors but have not been able to complete the task due to other priorities. Here are a couple of examples:

ginger:run hari$ python automaton.py
Traceback (most recent call last):
  File "automaton.py", line 347, in <module>
    CheckInput(ExpectedControl, ExpectedScheme, ExpectedFlow, ExpectedOutputControl, ExpectedResidualControl, Control, Scheme, Flow, OutputControl, ResidualControl)
  File "automaton.py", line 187, in CheckInput
    assert all(variable in ExpectedOutputControl['In'] for variable in OutputControl['In'])
AssertionError
ginger:LidDrivenCavity hari$ python automaton.py
Traceback (most recent call last):
  File "automaton.py", line 340, in <module>
    CheckInput(ExpectedControl, ExpectedScheme, ExpectedFlow, ExpectedOutputControl, ExpectedResidualControl, Control, Scheme, Flow, OutputControl, ResidualControl)
  File "automaton.py", line 183, in CheckInput
    assert len(next(os.walk(GridDir))[2]) == NumberOfBlocks
StopIteration
jayten commented 5 years ago

First error:

The automaton.py in the run directory is a sample script file. This script file is not supposed to be executed as the inputs are randomly provided as an explanation for the variables. Users are supposed to copy this script and modify as according to need. In the Tutorial subfolder, different automation.py file is provided with specific inputs.

Second error:

The assertion error indicates that the number of blocks provided as input in the file (NumberOfBlocks) does not match with the number of grid files available in the GridDir. So, grid files are not created before calling setup script automaton.py. This assertion helps to avoid the breakdown of the solver after it is executed. Each block in the solver requires an individual grid file.

As described in the documentation, the grid files for Lid-driven cavity test case can be created easily by just switching to the CreatedBlock subfolder and executing make command. An important point to note here is that the grid generation requires numpy version 1.10 or above for Python2.7. Once the grids the generated, the error should not occur.

Try following steps in the LidDrivenCavity folder:

$ cd CreateBlocks/
$ make

An error might occur if "numpy" version is less than 1.10 as the function "stack" is not implemented in the older versions.

$ cd ../
$ python automaton.py

Another commonly occurring mistake is not defining the path of the AbsBinaryPath. All the other input in the automaton.py will work without changing, but AbsBinaryPath is the absolute path to the directory where FEST-3D binary is compiled on your local computer.

In the automaton.py file make sure you change following: AbsBinaryPath="/Path/to/FEST-3D/bin/FEST3D"

More details can be found in the Documentation

Kevin-Mattheus-Moerman commented 5 years ago

@rouson thanks again for helping us review this JOSS submission. Can you please update us on your progress with this review? If you need more time please inform us as well. Thanks!

Kevin-Mattheus-Moerman commented 5 years ago

@hariradh do the comments by @jayten help you trouble shoot those errors?

hariradh commented 5 years ago

I cannot find any unit tests or API documentation. @jayten, could you please let me know if there are any.

Other than that, there should be at least one case like the LDC which needs more clearer documentation (like that provided above) to explain how to run a test case for validation.

jayten commented 5 years ago

@hariradh I will get back on your query in a few days. We are working on it.

labarba commented 5 years ago

👋 @rouson — Do you think you can complete this review within the next week or two?

jayten commented 5 years ago

@hariradh

API Documentation

The FEST-3D website provides detailed documentation of different source files, modules, procedures, and main program. This documentation is generated using FORD documentation generator. You can follow the links to find information on all the subroutines and functions written in different modules.

Unit Tests

It is rather difficult to write unit tests for the FEST-3D code; therefore, we have written integrated tests instead. FEST-3D repository has been updated with a new folder: test/. Please pull the changes in the master branch on GitHub. A python script, "Test.py," provided in the test/ directory to run the integrated tests with a particular flux scheme, higher-order method, and turbulence model. Use the following command to run all tests in the test/ directory. After generating the binary, you can run the test script using the following command in the test/ folder: 

$cd <FEST-3D/root/folder>/test/
$python Test.py <arg1>  <arg2>  <arg3>
  1. : Flux scheme - allowed options: - ausm - ldfss0 - ausmUP - ausmP - slau
  2. : Higher-order method - allowed options: - muscl - ppm - weno
  3. : Turbulence model - allowed options: - sst - sst2003 - kkl - sa

Examples:

  1. $python Test.py ausm muscl sst
  2. $python Test.py slau weno sa
  3. $python Test.py ausmUP ppm kkl

Please note that any flux-reconstruction scheme/higher-order method/turbulence model added by a developer can also be tested using Test.py script by suitably modifying the arguments passed to the script.

Once the tests are complete, you can check the summary in the Report.txt file. For more details about the integrated tests, please go through the TEST webpage in the FEST-3D documentation.

How to run FEST-3D

Also, as requested by @hariradh, we have updated the website with a new page on "How to run FEST-3D." This page provides a step-by-step approach on how to run the Lid-driven cavity test case.

rouson commented 5 years ago

@labarba Yes, I believe I can complete my review within a week of today.

rouson commented 5 years ago

@labarba @Kevin-Mattheus-Moerman I'm attempting to complete my review but finding that I'm unable to check any of the checkboxes on my checklist. Is this this submission still under review?

labarba commented 5 years ago

You mean, while logged in to GitHub, you click on the box, and nothing happens? It could be a permissions issue. Though you should have been automatically added to the right team when added as a reviewer. @arfon : could you check this?

arfon commented 5 years ago

@rouson - you need to accept this invite: https://github.com/openjournals/joss-reviews/invitations

rouson commented 5 years ago

@arfon - that did it. Thanks!

hariradh commented 5 years ago

@whedon accept

whedon commented 5 years ago

I'm sorry @hariradh, I'm afraid I can't do that. That's something only editors are allowed to do.

hariradh commented 5 years ago

@labarba I recommend that with the latest changes and documentation provided by @jayten, the paper be accepted.

labarba commented 5 years ago

👋 @rouson — what is your ETA for the review? (we already have a recommendation from the other reviewer). Thanks!

danielskatz commented 5 years ago

👋 @rouson - checking with you on your review again...

labarba commented 5 years ago

I have sent @rouson an email now.

rouson commented 5 years ago

@labarba thanks for the email. I somehow got subscribed to every JOSS communication so these messages got buried in the resulting flood. Hopefully I just resolved it by clicking "Unwatch." I'll work on my review later today and post the results.

labarba commented 5 years ago

👋 @rouson — how are you getting on with this review? Please give us an update.

rouson commented 5 years ago

@labarba I have added two issues that need to be addressed in order for me to complete my review. In addition, I am concerned about the appropriateness of releasing new software based on a language version that is very nearly three decades old (Fortran 90) and using a parallel programming model that is over two decades old (MPI-1). The Fortran standard has undergone 4 revisions since Fortran 90, each of which has added features that could be used to make the authors' code considerably more clear and concise and could potentially have benefits in robustness and performance. MPI has undergone two revisions since MPI-1 and more importantly, the most recent Fortran standards provide a sufficiently rich set of parallel features that it's not even clear there is a need for embedding MPI directly in new projects. The compiler and/or parallel runtime library can generate the MPI and would likely generate more sophisticated MPI (or an alternative parallel programming model) than scientific application developers would likely write so again there's the possibility for greater clarity, conciseness, robustness, and performance.

rouson commented 5 years ago

@labarba @danielskatz I just added another issue on this submissions repository with an 10-item list of modernization suggestions. Despite the list's length, it is surely not exhaustive as I've only reviewed a few files. I'd like your guidance on whether archaic use of the language is a relevant review criterion. I can't see where it fits into any of the items in my reviewer checklist, but after writing this latest issue, I would recommend rejection unless you're of the opinion that my concern is not part of the allowable criteria. I feel strongly that it's time for the world to move on from coding to a language standard that is very nearly 3 decades old. The code could definitely be more concise and robust and might even be higher performing if it were extensively overhauled and modernized to exploit the Fortran 2018 features that are supported by the compiler recommended in the installation instructions (gfortran). In a much more modern form, the code could be a great contribution to the literature.

rouson commented 5 years ago

I just added yet another issue as well as a lengthy comment on a previous issue. The more I look at the code, the more confident I am in recommending rejection. I'm certain this code addresses an important problem and is therefore useful, but it really needs a major revision to come up to the standards of what I would expect for any modern open-source package.

labarba commented 4 years ago

@rouson --- Thank you very much for the detailed recommendations for improvements that you have provided to the authors.

Note, though, that the JOSS review process seldom leads to a rejection, in the manner of traditional journals. The Review Guidelines, in the section on What happens if the software I’m reviewing doesn’t meet the JOSS criteria? says:

We ask that reviewers grade submissions in one of three categories: 1) Accept 2) Minor Revisions 3) Major Revisions. Unlike some journals we do not reject outright submissions requiring major revisions - we’re more than happy to give the author as long as they need to make these modifications/improvements.

In other words, we aim to help authors improve their software until it passes the bar to be accepted. They can, of course, decide that they are not willing to do the improvements, and request to withdraw the submission.

In this particular case, your critique of the software will need to be assessed by the authors, and they are allowed to come back to us with a proposal of what / how much they want to implement of your suggestions. The authors are not required to use particular languages or language features. JOSS reviewers/editors may insist for updates in the case of languages no longer supported (or soon to be), like Python 2.x. That is not the case with old Fortran, which is very much still used and supported.

It is the case, however, that using old languages may make the software less likely to be reused, less likely for others to contribute to it, and less likely to be cited. This is not grounds for rejection, but should be of keen interest to the authors.

I encourage the authors to assess the recommendations, and the improvements they would like to make. For example, this critique:

“hundreds of lines of code blocks comprised of loops nested 4 levels deep for no purpose other than computing a sum that could presumably be computed in a single line of code by invoking the sum function.”

... it sounds like a sizable reduction in number of lines of code is possible with some modernizing. Yet, the authors could have concerns with compilers available in their machines supporting the modern features.

@jayten — Please discuss with your team, and get back to us with your assessment.

rouson commented 4 years ago

@labarba I love that and fully support everything you wrote. JOSS is a breath of fresh air!

jayten commented 4 years ago

We are thankful to the reviewer @rouson for his insightful comments. Our team is not familiar with the latest Fortran version, but based on the reviewer's suggestion, it seems like the right way forward to use newer features supported by the latest Fortran version. However, at the same time, we want to make sure that It does not create any conflict with our already planned work - implementation of OpenACC directive based parallelisation of loops. As Gfortran9 only provides experimental and incomplete support for OpenACC, the new members of our team have chosen the PGI compiler to write directive-based GPU programming. As such, Coarray seems to be very simple and productive to use, the PGI compiler appears to be holding back in supporting Coarray as they are still only FORTRAN 2003 compliant. We are also not sure if array operation is compatible with OpenACC as it seems from our preliminary reading that OpenACC directives recognise the loops (which are to be run parallel in GPU) from the "do" and "end do" statements. We are still new to this field (use of OpenACC directives) and will be happy to be corrected if we are wrong.

Since most of the other suggestions by the reviewer are doable without any conflict with our future work, we propose to do the following work for now:

  1. Reducing the length of the code wherever possible (for eg: a single subroutine for all three directions of the 3D grid, use of derived datatype, etc. )
  2. Making variables local to the module.
  3. Instead of using just global variables, passing arguments to subroutines.
  4. Write unit tests with ctest.
  5. Remove unnecessary comments and files from the code.

A point to note: the portion of code that the reviewer suggested could be replaced with just a sum statement is actually a buffer collection operation, wherein it is then passed across nodes using MPICH MPISend and MPIRecv commands. We realise though that with the use of Coarrays this will be just a few lines of code.

labarba commented 4 years ago

👋 @jayten — Do let us know what your timeline looks like for the work you have proposed to do. Best wishes for the New Year! 🎆

jayten commented 4 years ago

@labarba The proposed work has been completed. The changes have been tested and merged with the master branch. Please review the changes here.

The summary of the changes made is:

  1. Added install bash script for automatic installation of FEST-3D with dependencies on Ubuntu, CentOS, and MacOS platforms. This script will install dependencies, build executable, run unit, and integrated test.
  2. Added Travis CI.
  3. Since Python2 will no longer be maintained past 2020, Python script has been moved from version 2 to version 3
  4. Clustering similar variables using derived data-types, which helps in making the argument list of various subroutines shorter.
  5. Instead of using global variables, most variables are passed as subroutine arguments.
  6. Updated CMake to automatically include for MPICH library, which removes the need to specify Fortran compiler while executing CMake file.
  7. Integrated Ctest for Fortran with CMake to avoid additional dependencies for unit test setup.
  8. Added 25 unit tests.
  9. Removed commented code.
  10. Deleted unnecessary archived/unused files.
  11. Update documentation with the above changes.

Additionally, the install script also sets the "FEST3D" environment variable which will avoid the error faced by both reviewers while setting up the test cases using Python script in the "run/" directory.

labarba commented 4 years ago

👋 @hariradh, @rouson — The authors of this submission have made substantial improvements, and we ask that you take a look a this work and advice on the state of the software now. If you are ready to recommend this for publication, please let us know. Thank you!

rouson commented 4 years ago

@labarba This is a very impressive step forward. I recommend acceptance. I just installed FEST-3D on macOS and had a much better experience with the process this time. After I issued the one simple installation command provided in the repository's README.md, the installation completed without problem, including running and passing all 25 unit tests and all 3 integration tests. Ease of installation greatly improves the user experience and passing tests greatly improve user confidence.

I also looked at several source files and believe the code at least makes better use of Fortran 90. I encourage the authors to explore and exploit the numerous major advances made in more recent Fortran standards, including support for object-oriented programming, parallel programming, and functional programming. I suspect the code could still be made quite a bit less verbose through the use of array statements and more robust by either eliminating pointers or incorporating reference counting. At a minimum, all pointers should be default-initialized to null() so that no pointer starts life in an undefined state. I hope to find the time to make more suggestions via pull request.

labarba commented 4 years ago

@rouson — Are you ready to tick off the remaining items in your review checklist?

labarba commented 4 years ago

@hariradh — I see all your checklist items are ticked off. Can we have a confirmation here that you recommend acceptance?

hariradh commented 4 years ago

I confirm that I recommend acceptance.

On 3 Feb 2020, at 3:01 PM, Lorena A. Barba notifications@github.com wrote:

@hariradh https://github.com/hariradh — I see all your checklist items are ticked off. Can we have a confirmation here that you recommend acceptance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/1555?email_source=notifications&email_token=AAUO52BFYRATVA7H6M3IZADRBAIT3A5CNFSM4H7EZ65KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKTYC3A#issuecomment-581402988, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAUO52DCBXQJKJVY7GDMVWLRBAIT3ANCNFSM4H7EZ65A.

rouson commented 4 years ago

@labarba I just checked off all remaining boxes on my reviewer checklist.

labarba commented 4 years ago

@whedon generate pdf

whedon commented 4 years ago

:point_right: Check article proof :page_facing_up: :point_left:

labarba commented 4 years ago

Please merge https://github.com/FEST3D/FEST-3D/pull/11 which provides small typo fixes.

Then, you need to do these final steps:

You may find this helpful: https://guides.github.com/activities/citable-code/

jayten commented 4 years ago

I have released the reviewed FEST-3D software as a new version with tag v1.1 and archived the same on Zenodo with DOI: 10.5281/zenodo.3660408 .

labarba commented 4 years ago

@whedon set v1.1 as version

whedon commented 4 years ago

OK. v1.1 is the version.