search

openjournals / joss-reviews

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

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

Closed whedon closed 7 years ago

whedon commented 8 years ago

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

Paper PDF: 10.21105.joss.00064.pdf

arfon commented 8 years ago

/ 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

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

rmflight commented 8 years ago

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 commented 8 years ago

@rmflight yes code requiring a proprietary run-time may be eligible. See also the discussion we had on this here: https://github.com/openjournals/joss/issues/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 commented 8 years ago

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 https://github.com/openjournals/joss/issues/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 https://github.com/openjournals/joss-reviews/issues/64#issuecomment-247741507, or mute the thread https://github.com/notifications/unsubscribe-auth/ABcI-lJKxdzQlmJ7n31Dw-Wwn3d8N98Zks5qq0OLgaJpZM4J_RJO .

arfon commented 8 years ago

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 commented 8 years ago

@whedon assign @Kevin-Mattheus-Moerman as editor

whedon commented 8 years ago

OK, the editor is @Kevin-Mattheus-Moerman

Kevin-Mattheus-Moerman commented 8 years ago

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

kakearney commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

tpoisot commented 8 years ago

If it runs in Octave, yes.

roliveros-ramos commented 8 years ago

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

Kevin-Mattheus-Moerman commented 8 years ago

@whedon assign @roliveros-ramos as reviewer

whedon commented 8 years ago

OK, the reviewer is @roliveros-ramos

Kevin-Mattheus-Moerman commented 8 years ago

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

Kevin-Mattheus-Moerman commented 8 years ago

@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 commented 8 years ago

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

kakearney commented 8 years ago

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 https://github.com/openjournals/joss-reviews/issues/64#issuecomment-249300370, or mute the thread https://github.com/notifications/unsubscribe-auth/AEyGNACzvWSbhv7SZJU3zwUhkGlT35OTks5qtD0NgaJpZM4J_RJO .

Kevin-Mattheus-Moerman commented 8 years ago

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

Kevin-Mattheus-Moerman commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

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 https://github.com/openjournals/joss-reviews/issues/64#issuecomment-251981673, or mute the thread https://github.com/notifications/unsubscribe-auth/AEyGNN6CpZlLtqxJEOOys5ZdfeY-_3Urks5qxQc7gaJpZM4J_RJO .

roliveros-ramos commented 8 years ago

@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 commented 8 years ago

There are a few key Matlab features that this software relies on that limit its back-compatibility:

The last of those will only limit the use of the ecopathmodel graph method, which is really a bonus feature, not necessary to the core calculations. So I believe this code should be mostly functional under R2014a/b. It will not work properly in R2012.

As I note at the end of the README, this class-based, table-based version of the code is relatively new. I decided a reliance on newer Matlab features was worth it to create more user-friendly (and input validation-friendly) storage of the hundreds of parameters in an Ecopath model. In general, I'm a big proponent of maintaining back-compatibility, and so I did not make the decision to switch to this format lightly; the use of table arrays streamlined both use and maintenance of the code, which had gotten increasingly difficult as I developed this code from a limited, project-specific implementation to a full Ecopath replication useable by others.

Please note that the old version is still available, and the primary ecopath calculation in that code set (ecopathlite.m) uses nearly identical code to the ecopathmodel ecopath method in this one. That version can be run in Matlab as old as 2007a, I believe (I make ample use of the bsxfun function, which was introduced at that time). However, I am not actively maintaining that older version anymore.

Kevin-Mattheus-Moerman commented 7 years ago

@roliveros-ramos Were you able to upgrade MATLAB to review this submission? Let me know if there are any issues.

Kevin-Mattheus-Moerman commented 7 years ago

@roliveros-ramos are you still able to review this submission? Your help would be greatly appreciated.

jscamac commented 7 years ago

Sorry I was away when I was originally tagged in this message, and I forgot to respond. I don't use Matlab so probably wouldn't be of use. J

roliveros-ramos commented 7 years ago

@Kevin-Mattheus-Moerman, I'm sorry for the lack of news. It's been dificult to check the code. I got only MATLAB 2014a for Windows, but I never managed to install the mdbtool library. I use Fedora but don't have MATLAB for linux available. I do have C compilers available but I think the installation process for MS Windows is not as simple as described (not described, actually), mainly for the dependencies of mdbtools itself. After several tries I gave up. I checked the code that doesn't need to use mdbtools and it's working fine. I tried to run the rest of the code with other model included in the demo (Esa) and most of it works properly (despite lots of warnings) so I'd guess it should work fine with the proposed model.
As a programmer and ecopath user I've dreamed with a tool like this for a lot of time, but my current opinion is an OS restriction should be added because it's not really functional for Windows, which is a pitty since EwE is provided for Windows only and most of the potential users use Windows. Even if for production use may be use in servers running linux (E2E models?), the added value of reproducibility is still appealing for regular EwE users which relay on Windows for their work. In summary, I cannot fully answer the section of functionality, and in the current state the installation instructions are not available for Windows users. Minor stuff: Christensen and Walters 2004 doesn't include a DOI.

Kevin-Mattheus-Moerman commented 7 years ago

@jscamac That is okay. Thanks for your response.

Kevin-Mattheus-Moerman commented 7 years ago

@roliveros-ramos thank you so much for your patience and hard work. If you have a MATLAB license associated with your MathWorks account you can likely download the linux version also. Although the download and installation would require quite a lot of your time.... If the authors are able to provide proper instructions for MS Windows, would you be willing to continue the review process at that point? This would be greatly appreciated.

@kakearney can you address @roliveros-ramos 's comments? Can you consider:

If your software is not meant for MS Windows users please clarify this in the documentation/instructions and the paper.

kakearney commented 7 years ago

Response to reviewer:

I got only MATLAB 2014a for Windows, but I never managed to install the mdbtool library. I use Fedora but don't have MATLAB for linux available. I do have C compilers available but I think the installation process for MS Windows is not as simple as described (not described, actually), mainly for the dependencies of mdbtools itself. After several tries I gave up.

The limitation referred to here is related to the mdb2ecopathmodel.m function, one of several tools for importing model data into an ecopathmodel object from other sources.

To clarify, this limitation does not affect the core capabilities of this code package. The ecopathmodel class is intended to be the focus, providing both data storage (in class object properties) and a variety of Ecopath-related calculations (in the class methods), and the class can be used completely independently of any external compiled software.

That said, I think the ability for a researcher to move data between the different flavors of Ecopath (the original EwE GUI, Rpath, EwE-F, and my ecopath_matlab) without manual typing or copy-pasting is important, and one of the key features that makes my entry stand out from the crowd. In addition to being able to create and populate an ecopathmodel object manually from Matlab, users can import from EwE-F text files, Rpath-style .csv files, or EwE6 .EwEmdb files. The latter one is the troublemaker, and unfortunately also the one most users will want to use the most (since in the Ecopath world, 99% of users build their models using the EwE software). And given that, the lack of cross-platform support by the mdb2ecopath.m code is a very frustrating limitation.

A little history on this limitation and why I haven't figured out a universal solution yet... The original Ecopath with Ecosim software was written using Visual Basic; starting with EwEv6, it was ported to Visual .NET. For data storage, the authors chose to use Microsoft Access database, i.e. .mdb, files. This is a proprietary, not-publically-documented file format, and there are no native Matlab functions to read this data. Therefore, I currently rely on a third-party piece of software, known as mdbtools (https://github.com/brianb/mdbtools), which allows one to dump .mdb tables into simple .csv files, and from there I read them into Matlab. The mdbtools library is well-documented for *nix systems, but not at all for Windows systems (I assume this is because the target audience is those without access to any Microsoft Access drivers, i.e. non-Windows users). Based on the discussion threads in the mdbtools GitHub repo, it seems that the library can be compiled on Windows. However, my Windows use is currently limited to a very small virtual machine, and I have not personally accomplished this Windows compilation. I am currently investigating workarounds for mdb-reading on Windows using the Matlab Database Toolbox; if offers some utilities to use a locally-installed ODBC driver (available only on Windows) for the task. I should be able to acquire said ODBC driver for my VM shortly, and hope to rectify this limitation in direct read of .Ewemdb files on a Windows OS as soon as possible. However, even that solution will be pretty limited, since it requires users to also have the ODBC driver (common for Windows users, I think?) plus a separate Matlab Database Toolbox license (not so common toolbox for oceanographers and fisheries scientists, and certainly not free to add).

I'll note that I state pretty clearly in the tutorial (under "Importing Ecopath with Ecosim data") that the lack of MS Windows support for mdb2ecopathmodel.m is a known shortcoming, one I recognize as affecting many users, and one for which I am actively looking for workarounds. I also provide copies of both example example ecosystems in both EwE6 format (.mdb files) and Rpath .csv format. So while Windows users may not be able to work through the specific example that imports directly from the .EwEmdb file, they can still build the same ecopathmodel object using alternative import methods (rpath2ecopathmodel.m).

I am currently working on an additional import function to read EwE6 .csv files. This set of 13 .csv files can be exported directly from the EwE6 software; it's not ideal, since the files need to be exported manually one at a time (and re-exported if the user modifies the model data at all), but it will serve as a stopgap solution so users can get their data from EwE6 to Matlab without needing the mdbtools library, and without having to resort to copying and pasting. I will post here where that function is completed an added to the repo.

I tried to run the rest of the code with other model included in the demo (Esa) and most of it works properly (despite lots of warnings) so I'd guess it should work fine with the proposed model.

Could the reviewer clarify what he meant by "most" of the code working? Was there a particular part of the tutorial (besides direct import) that didn't work, or that provided different answers than expected?

Regarding the warning messages, I note in the tutorial that those messages are to be expected. One purpose of this rewrite of Ecopath is to provide as much transparency as possible, so any time my code alters a user-provided value, it issues a warning letting the user know. This includes any changes related to placeholder values. In the ESA model example, NaNs were used as placeholders for non-relevant parameters (for example, consumption rate of prey for primary producers that don't eat); the code changes these to 0s (to avoid propagation of NaNs in some later calculations), and the warning messages state clearly where this was done and why.

Each of the above warning messages are coded using standard Matlab warning identifiers, and can be silenced by the user if they so choose.

In summary, I cannot fully answer the section of functionality, and in the current state the installation instructions are not available for Windows users.

I appreciate the attempt to review, but I feel that this statement judges the entire code package based on a rather minor portion of it. I will work to correct the direct import on Windows limitation, but I believe the ecopathmodel class can stand on its own even without that feature. If necessary, I can recommend some additional reviewers to take a look at the code.

Minor stuff: Christensen and Walters 2004 doesn't include a DOI.

The DOI for that paper is 10.1016/j.ecolmodel.2003.09.003. I have added that information to the paper.bib file.

Kevin-Mattheus-Moerman commented 7 years ago

@kakearney thanks for your response. If you can suggest additional reviewers that would be great thanks.

kakearney commented 7 years ago

I don't want to post emails to this thread, so I've emailed a couple potential reviewers, and directed them to volunteer in this thread if they are willing.

On Mon, Nov 28, 2016 at 2:05 PM, Kevin Mattheus Moerman < notifications@github.com> wrote:

@kakearney https://github.com/kakearney thanks for your response. If you can suggest additional reviewers that would be great thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openjournals/joss-reviews/issues/64#issuecomment-263409497, or mute the thread https://github.com/notifications/unsubscribe-auth/AEyGNMi3C-LX_VfpEeR45la7njR32PC3ks5rC1AsgaJpZM4J_RJO .

roliveros-ramos commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Kevin-Mattheus-Moerman commented 7 years ago

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

roliveros-ramos commented 7 years ago

@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 commented 7 years ago

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

kakearney commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

roliveros-ramos commented 7 years ago

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

Kevin-Mattheus-Moerman commented 7 years ago

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

arfon commented 7 years ago

@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 commented 7 years ago

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