Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REVIEW]: ecopath_matlab: A Matlab-based implementation of the Ecopath food web algorithm #64

Closed
16 tasks done
whedon opened this issue Sep 16, 2016 · 54 comments
Closed
16 tasks done
Assignees
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review

Comments

@whedon
Copy link

whedon commented Sep 16, 2016

Submitting author: kakearney (Kelly Kearney)
Repository: https://github.com/kakearney/ecopath_matlab-pkg
Version: v2.0
Editor: @Kevin-Mattheus-Moerman
Reviewer: @roliveros-ramos
Archive: 10.5281/zenodo.240977

Status

status

Status badge code:

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

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

  • 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

  • Repository: Is the source code for this software available at the repository url?
  • License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
  • Version: Does the release version given match the GitHub release (v2.0)?

Functionality

  • Installation: Does installation proceed as outlined in the documentation?
  • Functionality: Have the functional claims of the software been confirmed?
  • Performance: Have any performance claims of the software been confirmed?

Documentation

  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
  • Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
  • Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
  • Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
  • 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

Paper PDF: 10.21105.joss.00064.pdf

  • Authors: Does the paper.md file include a list of authors with their affiliations?
  • A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
  • References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?
@whedon whedon added the review label Sep 16, 2016
@arfon
Copy link
Member

arfon commented Sep 16, 2016

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

  • Please work through the checklist at the start of this issue.
  • If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
  • Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

@rmflight
Copy link
Member

I'm curious if a software package that depends on a licensed non-free product counts as open source software and is within the scope of the journal?

@Kevin-Mattheus-Moerman
Copy link
Member

@rmflight yes code requiring a proprietary run-time may be eligible. See also the discussion we had on this here: openjournals/joss#142. Following that discussion we now say the following here http://joss.theoj.org/about:

What about submissions that rely upon proprietary languages/development environments?
We strongly prefer software that doesn't rely upon proprietary (paid for) development environments/programming languages. However, provided your submission meets our submission requirements (including having a valid open source license) then we will consider your submission for review. Should your submission be accepted for review, we may ask you, the submitting author, to help us find reviewers who already have the required development environment installed.

And at the reviewer guidelines:

What about submissions that rely upon proprietary languages/development environments?
As outlined in our author guidelines, submissions that rely upon a proprietary/closed source language or development environment are acceptable provided that they meet the other submission requirements and that you, the reviewer, are able to install the software & verify the functionality of the submission as required by our reviewer guidelines.
If an open source or free variant of the programming language exists, feel free to encourage the submitting author to consider making their software compatible with the open source/free variant.

@rmflight
Copy link
Member

I apologize, that was an oversight on my part. I should have looked at the
reviewer guidelines. Thank you for drawing my attention to that.

On Fri, Sep 16, 2016 at 9:34 PM Kevin Mattheus Moerman <
notifications@github.com> wrote:

@rmflight https://github.com/rmflight yes code requiring a proprietary
run-time may be eligible. See also the discussion we had on this here:
openjournals/joss#142 openjournals/joss#142.
Following that discussion we now say the following here
http://joss.theoj.org/about:

What about submissions that rely upon proprietary languages/development
environments?

We strongly prefer software that doesn't rely upon proprietary (paid for)
development environments/programming languages. However, provided your
submission meets our submission requirements (including having a valid open
source license) then we will consider your submission for review. Should
your submission be accepted for review, we may ask you, the submitting
author, to help us find reviewers who already have the required development
environment installed.

And at the reviewer guidelines:

What about submissions that rely upon proprietary languages/development
environments?

As outlined in our author guidelines, submissions that rely upon a
proprietary/closed source language or development environment are
acceptable provided that they meet the other submission requirements and
that you, the reviewer, are able to install the software & verify the
functionality of the submission as required by our reviewer guidelines.
If an open source or free variant of the programming language exists, feel
free to encourage the submitting author to consider making their software
compatible with the open source/free variant.


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#64 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABcI-lJKxdzQlmJ7n31Dw-Wwn3d8N98Zks5qq0OLgaJpZM4J_RJO
.

@arfon
Copy link
Member

arfon commented Sep 17, 2016

I apologize, that was an oversight on my part. I should have looked at the reviewer guidelines. Thank you for drawing my attention to that.

👍 no problem.

@arfon arfon changed the title Submission: ecopath_matlab: A Matlab-based implementation of the Ecopath food web algorithm [REVIEW]: ecopath_matlab: A Matlab-based implementation of the Ecopath food web algorithm Sep 20, 2016
@arfon
Copy link
Member

arfon commented Sep 21, 2016

@whedon assign @Kevin-Mattheus-Moerman as editor

@whedon
Copy link
Author

whedon commented Sep 21, 2016

OK, the editor is @Kevin-Mattheus-Moerman

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Sep 21, 2016

@kakearney Can the authors help to suggest reviewers for this submission?

@kakearney
Copy link

What is the journal's process for suggesting reviewers? Should I compile some suggested names and send them to you (the editor), or should I try to personally contact people and direct them to here to volunteer?

@arfonsmith
Copy link

Please direct them to volunteer here

On Sep 21, 2016, at 12:27, Kelly Kearney notifications@github.com wrote:

What is the journal's process for suggesting reviewers? Should I compile some suggested names and send them to you (the editor), or should I try to personally contact people and direct them to here to volunteer?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@Kevin-Mattheus-Moerman
Copy link
Member

@mdietze @roliveros-ramos @jscamac @tpoisot is this your cup of tea? Would you be willing to act as reviewer for this submission?

@tpoisot
Copy link

tpoisot commented Sep 23, 2016

If it runs in Octave, yes.

@roliveros-ramos
Copy link

It is, I'll be happy to do it.

@Kevin-Mattheus-Moerman
Copy link
Member

@whedon assign @roliveros-ramos as reviewer

@whedon
Copy link
Author

whedon commented Sep 23, 2016

OK, the reviewer is @roliveros-ramos

@Kevin-Mattheus-Moerman
Copy link
Member

@kakearney I recommend adding a statement in your paper or documentation on compatibility with Octave.

@Kevin-Mattheus-Moerman
Copy link
Member

@tpoisot Thanks for your quick response. I've asked the authors to clarify compatibility with Octave. I have just assigned @roliveros-ramos as reviewer 1. For the moment the form at the top here handles a single reviewer (we are expanding our system to easily handle multiple reviewers). If you want to review it as well (if compatible with Octave) that would be great. For you to contribute as a second reviewer you can leave comments here or send me a report via kevin.moerman at gmail.com. Thanks!

@mdietze
Copy link

mdietze commented Sep 23, 2016

Looks like you're covered, and I don't run MATLAB so I wouldn't have been helpful anyway.

@kakearney
Copy link

I've never worked with Octave myself, so I'll have to download it and run
some tests. I'll update the paper accordingly once I've had a chance to do
that.

-Kelly

On Fri, Sep 23, 2016 at 1:56 PM, Kevin Mattheus Moerman <
notifications@github.com> wrote:

@tpoisot https://github.com/tpoisot Thanks for your quick response.
I've asked the authors to clarify compatibility with Octave. I have just
assigned @roliveros-ramos https://github.com/roliveros-ramos as
reviewer 1. For the moment the form at the top here handles a single
reviewer (we are expanding our system to easily handle multiple reviewers).
If you want to review it as well (if compatible with Octave) that would be
great. For you to contribute as a second reviewer you can leave comments
here or send me a report via kevin.moerman at gmail.com. Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#64 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEyGNACzvWSbhv7SZJU3zwUhkGlT35OTks5qtD0NgaJpZM4J_RJO
.

@Kevin-Mattheus-Moerman
Copy link
Member

@mdietze yes, understood. Thanks for the quick reply!

@Kevin-Mattheus-Moerman
Copy link
Member

Kevin-Mattheus-Moerman commented Sep 23, 2016

@kakearney It is likely that it is not fully compatible. A basic statement saying that compatibility has not been confirmed could suffice too. But to do a very quick check for compatibility here are some tips. Identify the most complex MATLAB functions in your submission, or functions requiring a special toolbox, or functions that seem very new in MATLAB (I sometimes, somewhat unfairly, refer to Octave as MATLAB 2005). Then you can check if these functions are part of the default Octave distribution. I ask about adding statements on Octave since the open source community in some cases favors non-proprietary languages like Octave.

@Kevin-Mattheus-Moerman
Copy link
Member

@roliveros-ramos thanks again for agreeing to review this submission. Let me know if you have any questions on the review process. Feel free to leave comments to the authors here. Do you think you can process the review within 2 weeks?

@kakearney any developments on checking compatibility with Octave? Or could you add a statement that is it likely not and has not been checked.

@kakearney
Copy link

I added a short statement to the README.md file clarifying that this code
will not run in Octave... Octave doesn't support the modern-Matlab class
syntax, along with numerous smaller incompatibilities.

On Thu, Oct 6, 2016 at 7:35 AM, Kevin Mattheus Moerman <
notifications@github.com> wrote:

@roliveros-ramos https://github.com/roliveros-ramos thanks again for
agreeing to review this submission. Let me know if you have any questions
on the review process. Feel free to leave comments to the authors here. Do
you think you can process the review within 2 weeks?

@kakearney https://github.com/kakearney any developments on checking
compatibility with Octave? Or could you add a statement that is it likely
not and has not been checked.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#64 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEyGNN6CpZlLtqxJEOOys5ZdfeY-_3Urks5qxQc7gaJpZM4J_RJO
.

@roliveros-ramos
Copy link

@Kevin-Mattheus-Moerman Sorry for the late reply, I'm in a meeting and don't have MATLAB in this laptop. I will meet the deadline though.

@kakearney I used MATLAB heavily from 2005-2008, then I moved to R. I'm very interested in a non-GUI version of EwE, this was really needed. I have MATLAB 2012 installed, do I need to get a more recent version? I think we have 2014.

@kakearney
Copy link

kakearney commented Nov 28, 2016 via email

@roliveros-ramos
Copy link

@kakearney: I do understand mdbtools is required for the mdb2ecopathmodel function only and I don't know if I miss the purpose of the package or not, but your script showing the functionality of the package relies in the Gen37 and Tb models, and both of them requires the mdb2ecopathmodel function, so I doesn't matter. I do know it is possible to install mdbtools on MS Windows but sadly I couldn't do it and I couldn't find instructions on the internet helpful to me. You also have to consider the dependencies for mdbtools need to be installed too:

Installation: Does installation proceed as outlined in the documentation?
Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.

I cannot check those points. If the process is simple it will be of great value to have add them to the installation instructions since, as you said, most EwE users work with the mdb format and MS Windows, and most EwE users are not particularly computer-savvy. That said, with MS Windows and MATLAB 2014a I was not able to run the code, reason why I cannot fully check the items in the review I mentioned.

Despite that, I run the script doing Gen37=Esa and Tb=Esa with minor changes in the names of the species (e.g. for the bootstrap). The code run but I got errors with most of the plots. Most of them can be related to the 2014a version I'm using (missing histogram function and methods for 'digraph') but I'm not sure (Error using table/subsrefDot (line 117) Index exceeds matrix dimensions.), but I do understand the full code was not intended to be run with the Esa model. A quick fix can be including Gen37 and Tb as csv files so Windows users can test the core functionality of the package without the mdbtools dependency, but I would stress that it reduces the usability of the package for Windows users (myself have several EwE models in the regular mdb format I cannot inmediately try with your package in the current stage). So, please, take my review as the point of view of a EwE and Windows user, I just learned with your article about Rpath, for example.

@Kevin-Mattheus-Moerman: If the authors provide installation instructions for MS Windows or find an alternative solution for MS Windows users I'd like to continue with the review, I'd be able to borrow a MATLAB 2016a.

@kakearney
Copy link

Thank you for the additional clarification.

As I was preparing to respond to this comment, I received an email from a new ecopath_matlab user asking if I had a workaround for the mdbtools limitation on Windows. So while I may consider that function a bonus feature, it's now obvious to me that users need a solution to import existing EwE6 models directly on a Windows system. I will try to get the EwE6 .csv import option working as soon as possible, and will add this option to the tutorial. I think this should take me about a week.

The code run but I got errors with most of the plots. Most of them can be related to the 2014a version I'm using (missing histogram function and methods for 'digraph') but I'm not sure

The histogram function was introduced in Matlab R2014b (intended to replace hist and histc), and graph/digraph objects in R2015b. The histogram plot was simply demonstrating distribution of parameters; I'll go ahead and change that code to use a more back-compatible option.

but I do understand the full code was not intended to be run with the Esa model.

While the exact verbatim code in the tutorial was designed around several different models (Gen37 represents the simplest sort, Tampa Bay a more complex one with multi-stanza groups and multiple fishing fleets, and Esa a manually-built one with the sort of parameters often included in print journals), all the ecopathmodel methods should work with any set of model data.

A quick fix can be including Gen37 and Tb as csv files so Windows users can test the core functionality of the package without the mdbtools dependency, but I would stress that it reduces the usability of the package for Windows users (myself have several EwE models in the regular mdb format I cannot inmediately try with your package in the current stage)

The models are currently included in the examples folder in Rpath csv format. I'll add EwE6 csv format shortly (as well as instructions for exporting models from EwE6 into this format).

@kakearney
Copy link

As requested by the reviewer, I have now updated my submission so that the mdb2ecopathmodel.m function is able to be run on a Windows system.

When reviewing my options for the best method to import EwE6 data to Matlab on Windows systems, I considered 3 possibilities:

  1. Use Matlab's own database-reading functions to access a local ODBC driver and read data directly.
    Pro: direct read of data minimizes human error
    Con: These functions are relegated to the Database Toolbox, an add-on toolbox that is not commonly installed in fisheries and oceanography labs (e.g. not part of most NOAA lab licenses), and is not free to add.

  2. Use a python script (with the pyodbc module) to access a local driver, and call that python module from within Matlab.
    Pro: direct read of data minimizes human error, python is free
    Con: requires user to install python. Python is common among the physical/biogeochemical oceanography community, but less so in fisheries.

  3. Use EwE6's csv export option, then read in tables from .csv files.
    Pro: no third-party software required.
    Con: 13 tables need to be exported, one at a time, every time the user updates data in the EwE6 GUI. Group type and multi-stanza parameters are not able to be exported directly, so that info would need to be copied and pasted manually every time the model was updated.

I decided to go with option 2. I updated the mdb2ecopathmodel help section to include more detailed installation instructions for all operating systems. I also added more extensive error checking to the beginning of this function so that it scans for all the required third-party tools, and provides instructions for getting those set up if it doesn't find them.

In the overview example document, I added a more thorough explanation of these various direct-import options. I also clarified that the Rpath .csv format can be used as a backup in case direct read does not work, and pointed out the Rpath-formatted Gen37 and TampBay models that are available in the examples folder. Finally, I altered the example code to be back-compatible to R2014b, with a more specific error message regarding the one non-back-compatible method (@ecopathmodel/graph, which requires R2015b).

@Kevin-Mattheus-Moerman
Copy link
Member

@roliveros-ramos please can you pick up the review process again and consider the comments above by @kakearney

@Kevin-Mattheus-Moerman
Copy link
Member

@roliveros-ramos best wishes for the new year. Are you able to pick up the review process? Thanks.

@roliveros-ramos
Copy link

@Kevin-Mattheus-Moerman, I have finished the review. I really appreciate the efforts from @kakearney to make it easier to use to Windows users. The python functions works from MATLAB 2014b, so I changed from 2014a to 2016a, which is the MATLAB version I used to this review. I was able to fully run the demo included with the package and it's working as expected, I still have some comments:

  1. mdb2ecopathmodel function: I needed to change the mdbexport.py (line 33) to:
    DRV = '{Microsoft Access Driver (*.mdb, *.accdb)}'
    because the name of the driver in my computer was not the one provided by default. Without the proper driver name, you get the error:

python error: [im002] [microsoft][odbc driver manager] datasource name not found and no default driver specified

I'd be nice to have a warning or some workaround for that.

  1. ecopathmodel2rpath function. It works ok, but cannot be reverted using the rpath2ecopathmodel functions:
    Gen37 = rpath2ecopathmodel(fullfile('examples', 'Gen37', 'gen37'));
    since more files are required than the ones written by the function (e.g. Unable to open file 'examples\Gen37\gen37_pedigree.csv'.). I made it work creating empty "pedigree", "stanzas" and "stanza_groups" (just with the proper headers). The same applied to the TampaBay model.

  2. The headers of the file tb_stanza_groups.csv are incorrect, VGFB need to be changed to VBGF to make it work.

One additional comment which may be useful for the installation instructions: MS Office installed by default is the 32 bits one if you already have other 32bits MS Office stuff (any driver or viewer, very likely if you have your computer more than a few years for example), so normally ODBC drivers are 32 bits (or it was my case). If your MATLAB version is 64 bits you need to install Python 64 bits and have 64 bits ODBC drivers. You may have to reinstall some of the programs to make them compatible. I reinstalled MS Office 64bits since we have licenses for that. Not sure if it's the only solution, but was the simpler.

@Kevin-Mattheus-Moerman
Copy link
Member

Great work! Thanks a lot @roliveros-ramos ! @kakearney can you please address these additional comments?

@kakearney
Copy link

Thank you for the updated comments. I've addressed them as follows:

  1. I added a few additional lines of code to the mdbexport.py function to list all available drivers and choose one that included 'Microsoft Access Driver' in its name. That will at the least address the two versions of the driver name that I now know of. I'm not sure if that covers all possible options, but I will try to update this in the future if I discover other variations on the driver name.

  2. I changed the syntax of the rpath2ecopathmodel function to now match that of the read.rpath.params R function that it was designed to sort-of mimic. Under this syntax, only the base model table and diet table files are required; all others are optional, and assumed to be irrelevant if not explicitly included by the user. This unfortunately breaks back-compatibility with my old code, which I had been trying to avoid, but I think the new similarity in syntax between the R code and my Matlab code will make things much simpler for users trying to move between the two platforms. I updated the ecopathmodel_overview.m file to reflect this new syntax.

  3. Tracked this to a typo in the ecopathmodel2rpath.m function. I corrected it both there and in Tampa Bay .csv file.

@Kevin-Mattheus-Moerman
Copy link
Member

@roliveros-ramos can you review how @kakearney addressed your comments? Let me know if you have additional comments or if you feel this submission is now suitable for publication.

@roliveros-ramos
Copy link

I just pulled the lasted code and I'm getting this error:

Error using mdb2ecopathmodel>checkdeps (line 531)
Could not import mdbexport python module; please check that you either installed it locally or kept it in its default location in the ecopath_matlab package

Error in mdb2ecopathmodel (line 106)
checkdeps;

Points 2 and 3 are fine!

@kakearney
Copy link

Sorry, pushed that previous update without testing it; a small syntax error (spaces instead of tabs) prevented Matlab from properly importing the module. Just corrected that (and tested it on Windows this time)... should be good to go now.

@Kevin-Mattheus-Moerman
Copy link
Member

@kakearney great, looks like we are getting close. @roliveros-ramos can you please confirm?

@roliveros-ramos
Copy link

@Kevin-Mattheus-Moerman: everything looks fine now, I recommend the paper is accepted for publication.

@Kevin-Mattheus-Moerman
Copy link
Member

@roliveros-ramos great. Thanks for your awesome role as reviewer!
@kakearney congratulations! 🍰 We will process acceptance of your work shortly.

@arfon
Copy link
Member

arfon commented Jan 12, 2017

@kakearney - 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.

@arfon arfon added the accepted label Jan 12, 2017
@arfon
Copy link
Member

arfon commented Jan 12, 2017

BTW, here's the latest paper: 10.21105.joss.00064.pdf

@kakearney
Copy link

kakearney commented Jan 12, 2017

I just issued a new release, v2.1b. (Edit: accidentally copied and pasted the wrong one a moment ago, sorry about that). The new DOI is 10.5281/zenodo.240977

@arfon
Copy link
Member

arfon commented Jan 12, 2017

@whedon set 10.5281/zenodo.240977 as archive

@whedon
Copy link
Author

whedon commented Jan 12, 2017

OK. 10.5281/zenodo.240977 is the archive.

@arfon
Copy link
Member

arfon commented Jan 12, 2017

@roliveros-ramos many thanks for your review here and to you for editing @Kevin-Mattheus-Moerman

@kakearney - your paper is now accepted into JOSS and your DOI is http://joss.theoj.org/papers/10.21105/joss.00064 ⚡️ 🚀 💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted published Papers published in JOSS recommend-accept Papers recommended for acceptance in JOSS. review
Projects
None yet
Development

No branches or pull requests

10 participants