[REVIEW]: Habfuzz: A Fortran tool to calculate the instream hydraulic habitat suitability based on fuzzy logic

whedon commented 8 years ago

Submitting author: @chtheodoro (Christos Theodoropoulos) Repository: https://github.com/chtheodoro/Habfuzz Version: v1.0.0 Editor: @labarba Reviewer: @fonnesbeck Archive: https://dx.doi.org/10.5281/zenodo.163291

Status

Status badge code:

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

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) 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 questions

Conflict of interest

[x] As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).
General checks
[x] Repository: Is the source code for this software available at the repository url?
[x] License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
[x] Version: Does the release version given match the GitHub release (v1.0.0)?
Functionality
[x] Installation: Does installation proceed as outlined in the documentation?
[x] Functionality: Have the functional claims of the software been confirmed?
[x] Performance: Have any performance claims of the software been confirmed?
Documentation
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
[x] Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
[x] Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
[x] Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
[x] Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support
Software paper
[x] Authors: Does the paper.md file include a list of authors with their affiliations?
[x] A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
[x] References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?

fonnesbeck commented 8 years ago

Unless I am missing something, there is no clear specification of the platforms that the software is compatible with. It appears that it is MS-Windows-only, based on the fact that the installation instructions tell the user to "double-click habfuzz.exe to run the program".

If this is the case, I will not be able to review the paper, as I have no easy access to a Windows machine. It would be nice if the software were able to run on some sort of POSIX platform, but at a minimum the platform compatibilities need to be stated explicitly in the installation instructions, if not earlier.

chtheodoro commented 8 years ago

'Habfuzz has been developed to run on 32-bit and 64-bit Windows operating systems, including Windows XP, Vista, 7/8 and 10'. I have now inserted the above text at the 'instalation' section of the README.md

labarba commented 8 years ago

I am disinclined to accept in JOSS a piece of software that is Windows only. Although we (the JOSS editors) decided that open-source packages that require a proprietary runtime would be acceptable (reluctantly), that was a decision aimed at things like Matlab code. Software restricting the OS to Windows is in my opinion incompatible with the ethos of open source.

chtheodoro commented 8 years ago

I am not a software developer by profession, I am using Windows and developed this program in Windows. However, considering that the source files of fortran are read in every OS I suppose that if someone compiles the fortan files with a Linux/Unix-compatible Fortran compiler, it should work. And as I search for this I see it does. If it is ok with @labarba I will need a couple of days to write the Unix/Linux compilation instructions in the README.md.

fonnesbeck commented 8 years ago

The gfortran commands in the .bat file seemed to compile just fine, but it looks like the user interface would be the piece that would require some work, if I am interpreting this correctly.

chtheodoro commented 8 years ago

Habfuzz comes without a GUI. It inputs 3 .txt files and outputs 1 .txt file with the suitability in an array. I thought the lack of a GUI is not a problem... is it?

fonnesbeck commented 8 years ago

I assumed the .exe opened some sort of interface (not necessarily graphical), based on the documentation. No, there is no GUI requirement.

chtheodoro commented 8 years ago

Thank you @fonnesbeck. In Windows it opens the command prompt. I suppose in other OSs there is something similar. Please let me know if the review is still ongoing in order to update the README.md instructions. Sorry for any inconvenience caused...

fonnesbeck commented 8 years ago

I am not able to review this, as I do no not readily have access to a Windows machine, either at home or at work. Perhaps we can find someone who does.

chtheodoro commented 8 years ago

I was wondering if instead of not accepting, I could have some time to test the program and make it run in Ubuntu and Mac OS, with simultaneously updating the installation Information in the README.md...

arfon commented 8 years ago

I was wondering if instead of not accepting, I could have some time to test the program and make it run in Ubuntu and Mac OS, with simultaneously updating the installation Information in the README.md...

@chtheodoro that sounds like a great idea.

chtheodoro commented 8 years ago

Dear all,

Habfuzz has been now tested and runs on Windows, Ubuntu and MacOS X. I have provided the relevant instructions in the README.md. Another subroutine was also added and now the tool uses Fuzzy logic or Fuzzy Bayesian inference depending on the user's selection.

@arfon I have changed the README.md to provide only the basic information, while promtping the reader/user to consult the Habfuzz manual for details. Is this ok with you? I also changed the paper.md and the paper.bib, how can I apply these changes to the submission of JOSS?

chtheodoro commented 8 years ago

@labarba and @arfon is there anything else I need to do in order for the review process to continue?

labarba commented 8 years ago

@fonnesbeck : Are you able to review this submission on Ubuntu or OS X ?

fonnesbeck commented 8 years ago

Yes. Will get it done in the next few days.

fonnesbeck commented 8 years ago

First, thanks for making the code cross-platform!

Here are couple of specific notes:

misspelled "Brooks" on p.5 of manual
mcompile.sh is not executable when pulled from GitHub
the Geany text editor is cited as a dependency. Unless I am confused, there is no way for a text editor to be a dependency. You should restrict the dependencies list to things that are required for users to use the software.

The only major issue I see with the package is that there does not appear to be any unit tests to verify that the constituent functions in Habfuzz behave as expected. While there are test cases provided, this is not the same as having tests that will fail when important bugs are introduced to the code (either by developers or users).

labarba commented 8 years ago

@fonnesbeck Did you install the software, then? If installation went without problems, do check the relevant items on the checklist at the top of this issue. Are you satisfied with the statement of need? See other check items above.

fonnesbeck commented 8 years ago

I was able to install and run the software. The outstanding issues, based on the review criteria include:

paper.md seems to be incomplete. e.g. no references or author list. It appears much of the missing information could be moved from the README.
no automated tests

chtheodoro commented 8 years ago

Thank you @fonnesbeck for the quick reply!

I removed the 'Geany' text editor from the dependencies list. The paper.md is also updated with the reference list (the authors are already mentioned in the top box named 'authors'). Corrected 'Brookes' spelling also.

I am now working on the automated tests... I will let you know soon.

arfon commented 8 years ago

I removed the 'Geany' text editor from the dependencies list. The paper.md is also updated with the reference list (the authors are already mentioned in the top box named 'authors'). Corrected 'Brookes' spelling also.

@chtheodoro - you don't need to include the references in the References section of paper.md. Instead, you just need to list them in the paper.bib and cite the references inline in your paper body. See this paper as an example: https://github.com/paulcon/active_subspaces/tree/master/JOSS_Paper

chtheodoro commented 8 years ago

Automated tests (unit tests) are now added in the tests folder of the program, along with a small README.md. I also updated the paper.md and paper.bib according to @arfon 's indications.

I hope I didn't miss anything... Please let me know if we are ok now. Thank you!

@arfon as I told you before, I assume that the changes applied at the paper.md and paper.bib will be automatically passed into the final (hopefully) published pdf file, or not?

chtheodoro commented 8 years ago

@labarba and @fonnesbeck when you are able we can continue with the review process. I applied all of @fonnesbeck 's indications.

fonnesbeck commented 8 years ago

Thanks. I will check the tests as soon as I can.

fonnesbeck commented 8 years ago

@chtheodoro Unfortunately, I am having trouble installing the FRUIT testing framework. I have filed a bug report, but given that the last report is 2 years old, I'm not sure how soon it will get a response. If you have an idea of where things are going wrong, please let me know (I am not much of a Ruby expert).

BTW, you state that Ruby/rake are optional dependencies for testing, but it seems that FRUIT requires them for installation, so they do not appear to be optional.

chtheodoro commented 8 years ago

@fonnesbeck I had the same trouble on Windows. I updated the instructions and now it's ok to run. No need for FRUIT, Ruby and Rake,, just put the fruit.95 file from the repository together with the test file, the test driver file, the fdeclarations.f95 and the relevant subroutine to test, compile with gfortran and you should be able to run. I tried it on a Mac and it works.

chtheodoro commented 8 years ago

@fonnesbeck my apologies for bothering you again about this, but did you manage to run the tests by using only the fruit.f95 file according to the updated README instructions?

fonnesbeck commented 8 years ago

Can you create a script to automate the testing? Moving individual files around is not ideal for users or developers. What is the issue with running the tests in place?

chtheodoro commented 8 years ago

Each test calls a subroutine to test and I thought that the subroutine file should be in the same directory with the test in order to compile. But I assume I will find some way to automate the process ( through makefiles I guess)...

fonnesbeck commented 8 years ago

Yes, perhaps just a bash script to create a temporary directory, copy the files, run the test and clean up would be sufficient.

chtheodoro commented 8 years ago

@fonnesbeck I made the unit tests fully automated by using makefiles. You only need to write a specific command (see the README of the tests folder) and the test runs. Tested on both linux and os x!!

fonnesbeck commented 8 years ago

That seems to work, thanks.

The only outstanding item I see is that there are no references (with DOI) at the bottom of paper.md.

arfon commented 8 years ago

The only outstanding item I see is that there are no references (with DOI) at the bottom of paper.md.

@fonnesbeck - I need to compile the paper for you (this should pull in the references from paper.bib. I'll update the thread when I've done this.

arfon commented 8 years ago

@fonnesbeck here's the PDF: 10.21105.joss.00082.pdf

fonnesbeck commented 8 years ago

Looks good to me. Thanks for your patience and diligence in addressing my comments.

@labarba Everything checks out for me now.

labarba commented 8 years ago

Thanks ever so much, @fonnesbeck, for your diligent review. And good job, @chtheodoro, with responding to the reviewer requests.

@arfon : This submission can now be accepted in JOSS. Yay!

arfon commented 8 years ago

@arfon : This submission can now be accepted in JOSS. Yay!

✨ this is great!

@chtheodoro at this point could you make an archive of the reviewed software in Zenodo/figshare/other service and update this thread with the DOI of the archive? I can then move forward with accepting the submission.

chtheodoro commented 8 years ago

Thank you @fonnesbeck and @labarba !! @arfon the zenodo DOI is 10.5281/zenodo.163291.

I've notice that in the generated PDF, only the names are displayed in the references... Is there anything I did wrong in the paper.bib file? Normally, for example it should be (Mamdani and Assilian, 1975) but it is (E.H. and S., 1975).

chtheodoro commented 8 years ago

@arfon I finally managed to merge your branch with the master and everything seems ok now. Please let me know if there is anything else I should do.

arfon commented 8 years ago

10.21105.joss.00082.pdf

I've notice that in the generated PDF, only the names are displayed in the references... Is there anything I did wrong in the paper.bib file? Normally, for example it should be (Mamdani and Assilian, 1975) but it is (E.H. and S., 1975).

@chtheodoro is this looking better in the new compiled PDF above?

chtheodoro commented 8 years ago

Everything ok!!!

arfon commented 8 years ago

@fonnesbeck many thanks for the excellent review. @labarba ✨ for editing this one.

@chtheodoro your paper is now accepted into JOSS and your DOI is http://dx.doi.org/10.21105/joss.00082 🚀 🎉 💥

openjournals / joss-reviews