search

ReScience / ReScience-submission

ReScience submission repository
50 stars 97 forks source link

Review Request: T. Poisot #39

Closed tpoisot closed 6 years ago

tpoisot commented 6 years ago

AUTHOR

Dear @ReScience/editors,

I request a review for the following replication:

Original article

Title: On the coexistence of specialists and generalists Author(s): David Sloan Wilson and Jin Yoshimura Journal (or Conference): The American Naturalist Year: 1994 DOI: 10.1086/285702 PDF: http://www.journals.uchicago.edu/doi/abs/10.1086/285702

Replication

Author(s): Timothée Poisot Repository: https://github.com/PoisotLab/ReScience-submission PDF: https://github.com/PoisotLab/ReScience-submission/blob/poisot-2017/article/Poisot-2017.pdf Keywords: community ecology, specialist, generalist, coexistence, discrete time model Language: Julia v0.6 Domain: Ecology

Results

Potential reviewers

EDITOR

rougier commented 6 years ago

@tpoisot Thanks for your submission. An editor will soon be assigned. @karthik Can you edit this sumbission ?

🎉 First submission in Julia

rougier commented 6 years ago

@karthik Can you edit this submission ?

karthik commented 6 years ago

@rougier Yes, I'm happy to edit this.

karthik commented 6 years ago

👋 I've contacted both reviewers. Will update thread when I have a response and when the reviews begin.

pboesu commented 6 years ago

Looking forward to reviewing this submission!

rougier commented 6 years ago

@karthik Any progress on 2nd reviewer ?

karthik commented 6 years ago

@rougier Just sent a second reminder yesterday. Will start lining up others

pboesu commented 6 years ago

Hi @tpoisot and @karthik

This is a very nice and compact replication of the original article by Wilson and Yoshimura. The paper is clear, concise, and well written. The accompanying code is clear and well annotated. Most of the results from the original work are fully replicated (@tpoisot chose not to reproduce specific stochastic model runs presented by the original authors, which is fine as far as I'm concerned).

I only have some relatively minor suggestions, none of which are critical to the substance of the reproduction. Otherwise this is an accept pending minor revisions from my side.

  1. Random seeds: I'd love to get input on this from the author and editor(s), but I was wondering whether in the pursuit of full reproducibility it would be preferable to always use explicit RNG seeds for the stochastic simulations? As is, the code will reproduce almost, but not quite identical figures every time the manuscript is reproduced from scratch.

  2. Figure captions: The author clearly explains plotting symbols in the text, but the figure captions themselves are rather too terse. I'm a big fan of "stand-alone" figure captions, and would prefer if the plotting symbols were explained in the figure captions.

  3. Figure 3 axis labels: I personally find the format of the axis tick labels (fractional powers of 10) in Figure 3 unintuitive, and would prefer the use of integers as in Fig. 4 of the original study.

  4. Figure 4 z axis label: The z axis in Figure 4 is missing a label.

  5. Figure and Table cross references: Possibly an issue with templates in the markdown to PDF toolchain for ReScience, but figure and table cross references lack a label. E.g. https://github.com/PoisotLab/ReScience-submission/blame/poisot-2017/article/Poisot-2017.md#L151 prints as 'All default parameter values are given in 1.' rather than 'All default parameter values are given in Table 1.'

  6. I believe the reference for Pielou's evenness index J' is incorrect. The cited paper discusses the uses and misuses of the Shannon-Weaver diversity index, and makes no mention of evenness. The earliest derivation of J' I could find (in an non-exhaustive search) is in Pielou 1966 J Theor Biol 10:370-383 https://doi.org/10.1016/0022-5193(66)90133-0 .

tpoisot commented 6 years ago

@karthik -- I have revised the submission according to the comments of @pboesu. Specifically:

  1. I have added a fixed seed in the file where the core functions are defined. All figures use it.
  2. I have added the information on symbols to all legends.
  3. I have extended the axes so that most of them are integers.
  4. I apologize for the omission, this has been added.
  5. It is likely an issue with my setup, but I have added the Table and Figure in front of the references manually.
  6. This is correct -- I had exported the wrong Pielou 1966 reference, and this has been fixed. I apologize for the mistake.

The version of record for this revision is 4989845

dmcglinn commented 6 years ago

I apologize for missing these notifications. I'm happy to help with review if still needed.

rougier commented 6 years ago

@dmcglinn I think your review is still need (cc @karthik)

karthik commented 6 years ago

@dmcglinn No worries (I was also traveling). Please let me know if you are able to review (otherwise I can approach my next person in line). 🙏

dmcglinn commented 6 years ago

@karthik I'm happy to still help with this. I'll work on running the code early next week. I apologize again for the initial delay.

dmcglinn commented 6 years ago

I'm having trouble getting off the ground here with the code.

  1. I installed Juliapro which was one of the listed options on the main julia repo (https://juliacomputing.com/products/juliapro.html) the software installed but I can only run it locally (i.e., within the local install folder). I have tried adding that folder to my path but that has not solved the problem.

  2. Once I get julia up an running do I just run make in the code directory?

Thanks for any help you can provide.

dmcglinn commented 6 years ago

Ok I solved the first problem by exporting my path and then sourcing the .bashrc file. So I have julia installed and working on the command line now.

dmcglinn commented 6 years ago

Running julia ./figure02.jl rusulted in the following error:

ERROR: LoadError: LoadError: ArgumentError: Module Plots not found in current path.
Run `Pkg.add("Plots")` to install the Plots package.
Stacktrace:
 [1] _require(::Symbol) at ./loading.jl:435
 [2] require(::Symbol) at ./loading.jl:405
 [3] include_from_node1(::String) at ./loading.jl:576
 [4] include(::String) at ./sysimg.jl:14
 [5] include_from_node1(::String) at ./loading.jl:576
 [6] include(::String) at ./sysimg.jl:14
 [7] process_options(::Base.JLOptions) at ./client.jl:305
 [8] _start() at ./client.jl:371
while loading /home/dan/Documents/ReScience-submission/code/WY94.jl, in expression starting on line 3
while loading /home/dan/Documents/ReScience-submission/code/figure02.jl, in expression starting on line 1

`

dmcglinn commented 6 years ago

Ok I think I'm working through the julia dependency errors I'm working on recreating the figures now.

dmcglinn commented 6 years ago

@tpoisot I need help getting around this error: 

julia figure02.jl
WARNING: Couldn't initialize pyplot.  (might need to install it?)
INFO: To do a standard install of pyplot, copy and run this:

if !Plots.is_installed("PyPlot")
    Pkg.add("PyPlot")
end
withenv("PYTHON" => "") do
    Pkg.build("PyPlot")
end

# now restart julia!
...

I have previously run Pkg.add("PyPlot") and Pkg.build("PyPlot") both of which seemed like they worked.

dmcglinn commented 6 years ago

here is the full error message that is generated after running 

if !Plots.is_installed("PyPlot")
    Pkg.add("PyPlot")
end
withenv("PYTHON" => "") do
    Pkg.build("PyPlot")
end

and restarting julia I run 

julia> include("WY94.jl")

sys:1: UserWarning: 
This call to matplotlib.use() has no effect because the backend has already
been chosen; matplotlib.use() must be called *before* pylab, matplotlib.pyplot,
or matplotlib.backends is imported for the first time.

The backend was *originally* set to u'Qt5Agg' by the following code:
  File "/home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/backends/__init__.py", line 14, in <module>
    line for line in traceback.format_stack()

WARNING: No working GUI backend found for matplotlib
WARNING: Couldn't initialize pyplot.  (might need to install it?)
INFO: To do a standard install of pyplot, copy and run this:

if !Plots.is_installed("PyPlot")
    Pkg.add("PyPlot")
end
withenv("PYTHON" => "") do
    Pkg.build("PyPlot")
end

# now restart julia!

ERROR: LoadError: InitError: PyError (ccall(@pysym(:PyImport_ImportModule), PyPtr, (Cstring,), name)

The Python package matplotlib.pyplot could not be found by pyimport. Usually this means
that you did not install matplotlib.pyplot in the Python version being used by PyCall.

PyCall is currently configured to use the Julia-specific Python distribution
installed by the Conda.jl package.  To install the matplotlib.pyplot module, you can
use `pyimport_conda("matplotlib.pyplot", PKG)`, where PKG is the Anaconda
package the contains the module matplotlib.pyplot, or alternatively you can use the
Conda package directly (via `using Conda` followed by `Conda.add` etcetera).

Alternatively, if you want to use a different Python distribution on your
system, such as a system-wide Python (as opposed to the Julia-specific Python),
you can re-configure PyCall with that Python.   As explained in the PyCall
documentation, set ENV["PYTHON"] to the path/name of the python executable
you want to use, run Pkg.build("PyCall"), and re-launch Julia.

) <type 'exceptions.ImportError'>
ImportError("/lib/x86_64-linux-gnu/libz.so.1: version `ZLIB_1.2.9' not found (required by /home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/../../.././libpng16.so.16)",)
  File "/home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/pyplot.py", line 29, in <module>
    import matplotlib.colorbar
  File "/home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/colorbar.py", line 36, in <module>
    import matplotlib.contour as contour
  File "/home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/contour.py", line 21, in <module>
    import matplotlib.font_manager as font_manager
  File "/home/dan/.julia/v0.6/Conda/deps/usr/lib/python2.7/site-packages/matplotlib/font_manager.py", line 58, in <module>
    from matplotlib import afm, cbook, ft2font, rcParams, get_cachedir

Stacktrace:
 [1] pyerr_check at /home/dan/.julia/v0.6/PyCall/src/exception.jl:56 [inlined]
 [2] pyerr_check at /home/dan/.julia/v0.6/PyCall/src/exception.jl:61 [inlined]
 [3] macro expansion at /home/dan/.julia/v0.6/PyCall/src/exception.jl:81 [inlined]
 [4] pyimport(::String) at /home/dan/.julia/v0.6/PyCall/src/PyCall.jl:374
 [5] __init__() at /home/dan/.julia/v0.6/PyPlot/src/init.jl:184
 [6] _include_from_serialized(::String) at ./loading.jl:157
 [7] _require_from_serialized(::Int64, ::Symbol, ::String, ::Bool) at ./loading.jl:200
 [8] _require_search_from_serialized(::Int64, ::Symbol, ::String, ::Bool) at ./loading.jl:236
 [9] _require(::Symbol) at ./loading.jl:441
 [10] require(::Symbol) at ./loading.jl:405
 [11] _initialize_backend(::Plots.PyPlotBackend) at /home/dan/.julia/v0.6/Plots/src/backends/pyplot.jl:68
 [12] backend() at /home/dan/.julia/v0.6/Plots/src/backends.jl:185
 [13] #pyplot#264(::Array{Any,1}, ::Function) at /home/dan/.julia/v0.6/Plots/src/backends.jl:23
 [14] pyplot() at /home/dan/.julia/v0.6/Plots/src/backends.jl:23
 [15] include_from_node1(::String) at ./loading.jl:576
 [16] include(::String) at ./sysimg.jl:14
during initialization of module PyPlot
while loading /home/dan/Documents/ReScience-submission/code/WY94.jl, in expression starting on line 6
dmcglinn commented 6 years ago

Locally I'm still having trouble with the python version but on my server where I installed juliapro the code seems to be working except because my server is headless I'm getting the following error while trying to create graphics: 

julia figure02.jl
ERROR: LoadError: PyError (ccall(@pysym(:PyObject_Call), PyPtr, (PyPtr, PyPtr, PyPtr), o, arg, C_NULL)) <class '_tkinter.TclError'>
TclError('couldn\'t connect to display ":0"',)
  File "/home/mcglinnd/.local/lib/python2.7/site-packages/matplotlib/pyplot.py", line 586, in gcf
    return figure()
  File "/home/mcglinnd/.local/lib/python2.7/site-packages/matplotlib/pyplot.py", line 535, in figure
    **kwargs)
  File "/home/mcglinnd/.local/lib/python2.7/site-packages/matplotlib/backends/backend_tkagg.py", line 81, in new_figure_manager
    return new_figure_manager_given_figure(num, figure)
  File "/home/mcglinnd/.local/lib/python2.7/site-packages/matplotlib/backends/backend_tkagg.py", line 89, in new_figure_manager_given_figure
    window = Tk.Tk()
  File "/usr/lib/python2.7/lib-tk/Tkinter.py", line 1767, in __init__
    self.tk = _tkinter.create(screenName, baseName, className, interactive, wantobjects,useTk, sync, use)

Stacktrace:
 [1] pyerr_check at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyCall/src/exception.jl:56 [inlined]
 [2] pyerr_check at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyCall/src/exception.jl:61 [inlined]
 [3] macro expansion at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyCall/src/exception.jl:81 [inlined]
 [4] #_pycall#67(::Array{Any,1}, ::Function, ::PyCall.PyObject) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyCall/src/PyCall.jl:653
 [5] #pycall#71(::Array{Any,1}, ::Function, ::PyCall.PyObject, ::Type{PyCall.PyAny}) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyCall/src/PyCall.jl:675
 [6] gcf() at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/PyPlot/src/PyPlot.jl:150
 [7] _create_backend_figure(::Plots.Plot{Plots.PyPlotBackend}) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/Plots/src/backends/pyplot.jl:397
 [8] _plot_setup(::Plots.Plot{Plots.PyPlotBackend}, ::Dict{Symbol,Any}, ::Array{Dict{Symbol,Any},1}) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/Plots/src/pipeline.jl:213
 [9] _plot!(::Plots.Plot{Plots.PyPlotBackend}, ::Dict{Symbol,Any}, ::Tuple{Array{Float64,1},Array{Float64,1}}) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/Plots/src/plot.jl:202
 [10] #plot#145(::Array{Any,1}, ::Function, ::Array{Float64,1}, ::Vararg{Array{Float64,1},N} where N) at /home/mcglinnd/juliapro/JuliaPro-0.6.1.1/JuliaPro/pkgs-0.6.1.1/v0.6/Plots/src/plot.jl:56
 [11] (::RecipesBase.#kw##plot)(::Array{Any,1}, ::RecipesBase.#plot, ::Array{Float64,1},::Array{Float64,1}, ::Vararg{Array{Float64,1},N} where N) at ./<missing>:0
 [12] include_from_node1(::String) at ./loading.jl:576
 [13] include(::String) at ./sysimg.jl:14
 [14] process_options(::Base.JLOptions) at ./client.jl:305
 [15] _start() at ./client.jl:371
while loading /home/mcglinnd/ReScience-submission/code/figure02.jl, in expression starting on line 6
damiendr commented 6 years ago

For the problem on your local machine, see https://github.com/JuliaPy/PyPlot.jl/issues/151 I've never had any of these errors on MacOSX with a plain Julia install (also never tried Julia Pro), so I can't help much further...

tpoisot commented 6 years ago

@dmcglinn I'm using the stable version of Julia, not Julia pro. Have you installed matplotlib through pip?

rougier commented 6 years ago

@dmcglinn Did you make any progress ?

dmcglinn commented 6 years ago

I have not yet solved my install problems. I'm in the end of the semester crush but I hope to get to this soon. If you need to move on to a reviewer that is more familiar with Julia I would be fine with that.

On Mon, Dec 11, 2017 at 3:34 AM, Nicolas P. Rougier < notifications@github.com> wrote:

@dmcglinn https://github.com/dmcglinn Did you make any progress ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReScience/ReScience-submission/pull/39#issuecomment-350655545, or mute the thread https://github.com/notifications/unsubscribe-auth/ABIp7S4uLEdtYgP3cg7PqJJR9qeTC31Xks5s_OkjgaJpZM4P9_uB .

-- Daniel J. McGlinn, PhD Assistant Professor mailing address: Department of Biology College of Charleston 66 George Street Charleston, SC 29424 office address: Harbor Walk West, rm 203 360 Concord Street Charleston, SC 29401 http://mcglinn.web.unc.edu/ office: 843-953-0190 cell: 405-612-1780

karthik commented 6 years ago

@dmcglinn No worries. I understand the end of semester time crunches. If you are able to work through this after the semester ends (and after you enjoy some time off), that should be fine. But if you are truly stuck, please let me know and I will look for a new reviewer. We are grateful for all the time you've put into this.

karthik commented 6 years ago

👋 @dmcglinn Just a gentle ping to see if you had any luck with this.

dmcglinn commented 6 years ago

Hey @karthik and @tpoisot I am still working on this. I apologize for the long delay. I switched from working on this on a linux machine to a pc in hopes that maybe the install issues will resolve themselves. I have Julia 0.6.2 installed and added it to my path. I also have a local python with matplotlib. I still encountered many dependency errors but I was able to slog my way through them by installing each missing dependency. Unfortunately I have come to an error I cannot resolve. Upon running julia figure02.jl a runtime error window pops up (i.e., not a console error): error

If you have advice for how to proceed I can try again, otherwise I suggest you guys move on to a reviewer that is more versed in Julia and thus can better evaluate the project code rather than the difficulty in getting the software installed. Again my apologies on the slow progress. This was definitely more difficult than I expected it to be.

damiendr commented 6 years ago

This seems to be related to your issue: https://github.com/JuliaPy/PyPlot.jl/issues/334

rougier commented 6 years ago

@khinsen @dmcglinn Any progress ?

dmcglinn commented 6 years ago

I do not have more time to invest attempting to solve the julia problems. I suggest @karthik contact another potential reviewer. I apologize for holding up the review for this long. I never suspected that it would be this difficult to get the dependencies up and running.

rougier commented 6 years ago

@dmcglinn It's informative to know that Julia is not straightforward to run

khinsen commented 6 years ago

Beyond the immediate problem of completing the review, I see a longer-term issue of having code in a ReScience publication that is difficult to get to work. Cross-language interoperability beyond C APIs are notoriously fragile. Matplotlib on its own is a pain to install from source. Moreover, it adds other complex dependencies, such as Qt, even though they are not at all needed to run the submitted code that only produces plots in PDF files. So even if we find two reviewers that can get everything to work, future readers are likely to experience trouble.

I see basically two approaches to make this easier to run for everyone:

  1. Simplify the dependencies
  2. Provide a packaged runtime environment (e.g. as a Docker image), with instructions on how to use it for running the supplied code.

Since I don't have much trust in the logevity of Docker images, I personally prefer the first approach, but I don't the Julia ecosystem well enough to decide if it is doable with reasonable effort. Some ideas:

@tposoit, could you comment on this? BTW, I noticed that your platform info in README.md is incomplete: it mentions PyPlot.jl but not Python + matplotlib with the respective version numbers. Matplotlib in particular has changed quite a bit from 1.x to 2.x, breaking many plotting scripts.

khinsen commented 6 years ago

@rougier In my experience, plain Julia is hardly ever a problem, if you use the right version of Julia. It's the non-Julia dependencies that make life difficult.

tpoisot commented 6 years ago

@khinsen sure, I would love to comment on this.

@rougier I don't think it's fair to qualify julia of not being straightforward to run -- in my experience with the language on all 3 major OS, it has never been an issue. PyPlot.jl installs well when using conda as explained in the documentation. I'd like everyone not to generalize from issues of a single user on a single machine.

I have been running tests on a few different platforms in the meantime, and I have always been able to run the code. As far as I can tell, the issue is not with the package ecosystem but with a particular machine.

@karthik I will not change things unless someone else can reproduce the error that @dmcglinn had.

rougier commented 6 years ago

@tpoisot I'll give a try (but I've no experience with Julia)

pboesu commented 6 years ago

This review was my first use of julia. I installed it on my machine from scratch and ran it without any issues.

I'm not able to access that particular machine for the next few days, but I'm happy to report version numbers etc when I'm back in the office.

rougier commented 6 years ago

On OSX 10.13.3, using julia 0.6.2 (dmg install), I get:

julia> Pkg.add("Plots")
...
julia> Pkg.add("PyPlot")
...
julia> withenv("PYTHON" => "") do
           Pkg.build("PyPlot")
       end
...

Without the line below, julia complains about a missing package (ImportError('No module named mpl_toolkits.mplot3d',)) and part of the message was similar to the one from @dmcglinn 

export PYTHONPATH=/Users/rougier/.julia/v0.6/Conda/deps/usr

This path was given by julia during the PyPlot install. After that, works like a charm:

$ make
julia figure02.jl
julia figure03.jl
julia figure04.jl
julia figure05.jl
julia figure06.jl
julia figureSurf.jl
rougier commented 6 years ago

@tpoisot Sorry for jumping to wrong conclusions concerning Julia ease of use. @karthik Should we try to find a reviewer having access to Windows? And I realize this is an information we don't have and that we should probably collect it when reviewers are added.

rougier commented 6 years ago

@karthik Gentle reminder

karthik commented 6 years ago

Hi folks, apologies for the delays on my end. I was on my annual vacation and have just returned to email/internet. I've started looking for a new second reviewer and will update this thread shortly.

rougier commented 6 years ago

@karthik Any progress for a second reviewer?

rougier commented 6 years ago

@karthik Any progress?

karthik commented 6 years ago

@rougier I had two declines, waiting on a third person to reply.

ElOceanografo commented 6 years ago

Happy to provide a review, will try to turn it around quickly.

karthik commented 6 years ago

Thanks Sam!

ElOceanografo commented 6 years ago

This is a successful reproduction of the results in Wilson and Yoshimura. I was able to run the simulation code and generate the figures without any problems on my Windows machine, using a recent install of JuliaPro (v0.6.6.2). FWIW, I am an experienced Julia user who already had the tools installed and working, so my ease-of-use may not be totally representative. I did not attempt to run the Makefiles or build the document, since getting make , xelatex, etc. working on Windows would have been a significant headache. I can try doing those on an Ubuntu machine if needed.

The code and paper are well-written. The paper is clear and concise, though maybe a bit too concise in places. The Introduction would benefit from a bit more explanation as to why Wilson and Yoshimura's paper merits a replication attempt. Likewise, the Conclusion would be stronger if it touched on the motivation and significance of the original paper, and perhaps placed the current replication in the context of other work that has been based on Wilson and Yoshimura. This doesn't have to be an elaborate literature review, but another paragraph or so on the significance and context would be worthwhile.

A number of minor revisions follow; as long as these and my comments above are addressed I recommend the paper be accepted.

Text comments

Code comments

These are mostly just nitpicking...

rougier commented 6 years ago

@tpoisot @karthik Any progress on the review?

tpoisot commented 6 years ago

I have modified the text to answer @ElOceanografo comments. I have been unable to build the PDF on my laptop, but I can give it a try when I regain access to my desktop.

rougier commented 6 years ago

Sorry for the delay. Any progress with the manuscript build, do you need some help?

rougier commented 6 years ago

@karthik @tpoisot Any update?