ReScience / ReScience-submission

ReScience submission repository
50 stars 97 forks source link

Review Request: Caze Stimberg Girard #45

Closed rcaze closed 6 years ago

rcaze commented 6 years ago

AUTHOR Romain Cazé ; Marcel Stimberg ; Benoît Girard

Dear @ReScience/editors,

I request a review for the following replication:

Original article

Title: Non-additive coupling enables propagation of synchronous spiking activity in purely random networks
Author(s): R.M. Memmesheimer, M. Timme
Journal (or Conference): PLoS Computational Biology Year: 2012
DOI: 10.1371/journal.pcbi.1002384 PDF: http://journals.plos.org/ploscompbiol/article?id=10.1371/journal.pcbi.1002384

Replication

Author(s): Romain Cazé, PhD ; Marcel Stimberg, PhD; Benoît Girard, PhD
Repository: CAZE-STIMBERG-GIRARD PDF: Caze_2018.pdf Keywords: Dendrites; non-linearities; network; synfire chain Language: Python Domain: Neuroscience

Results

Potential reviewers


EDITOR

pietromarchesi commented 6 years ago

Hi, I'd be happy to edit or review this submission!

rougier commented 6 years ago

@rcaze Thanks for your submission, I'll edit it.

rougier commented 6 years ago

@pietromarchesi Thanks for your offer. I'll edit it, can you review it?

rougier commented 6 years ago

@rcaze Can you edit the first comment to indicate if this is a partial/full or failed replication ?

rougier commented 6 years ago

@degoldschmidt Can you review this submission?

pietromarchesi commented 6 years ago

@rougier Yes, sure!

degoldschmidt commented 6 years ago

@rougier Yes, of course!

rougier commented 6 years ago

@degoldschmidt @pietromarchesi Thank you. I think you can start the review.

pietromarchesi commented 6 years ago

I can't seem to find the environment.yml and requirements.txt files mentioned in the readme. As another minor note, the readme mentioned as Script directory that is not there anymore. Will begin the actual review hopefully this weekend.

mstimberg commented 6 years ago

I can't seem to find the environment.yml and requirements.txt files mentioned in the readme. As another minor note, the readme mentioned as Script directory that is not there anymore.

Apologies, these errors slipped in when we moved from our original repository to the ReScience repository. I added the missing files and fixed all mentions (I hope!) of incorrect directory names.

rcaze commented 6 years ago

Thank @mstimberg, my email notification wasn't working (targeted to an unused email address), good to see that the process is going forward. Maybe it is worth to mention that acknowledgement of reception are done this way.

rougier commented 6 years ago

@pietromarchesi The problem you mentioned seems to have been fixed, do you confirm ?

rougier commented 6 years ago

@pietromarchesi 🛎

pietromarchesi commented 6 years ago

Indeed. Working on the review!

pietromarchesi commented 6 years ago

Overall, the replications appears successful (within reasonable bounds) and the article is well written and well formatted (en passant, as a personal comment, I would like to mention that in some passages I found the explanation of the replication, which avoids some academic mannerisms, to be much clearer and to the point than the original paper).

I am currently running the --long simulation, which I will check on tomorrow. In the meantime, a few considerations in no particular order.

rougier commented 6 years ago

@pietromarchesi Thanks ! @degoldschmidt Did ou have a chance to review or do you need more time?

degoldschmidt commented 6 years ago

@rougier I am currently doing the code review, and should be done by the end of this week. Sorry for the delay.

pietromarchesi commented 6 years ago

The --long simulation also ran as expected in ~9 hours!

rougier commented 6 years ago

Ouch.

degoldschmidt commented 6 years ago

Reviewer 2

Dear @rcaze @mstimberg @benoit-girard, In summary, the manuscript describes the full reproduction within quantitative limits of the article by Memmesheimer/Timme (2012). I think that the authors describe the model intuitively and overall present a nicely written manuscript. The code is well written, however, I think it could contain more commenting. I successfully replicated the results described by the authors with minor differences for Fig. 2 (see below). However, I have one major issue and a few minor issues that I would like to address to the authors.

Major issue: First, while many of the decisions made by the authors have been explained, such as the choice of Brian and certain parameter adjustments, I am missing an explanation for why the authors decided not to use the original analytical solution as well. Furthermore, differences between this Figure (3) and the original that are described are too short. Instead of only replicating the figure and describing a "good match", I would like to see a more quantitative comparison (e.g., position of the maximum, intersections, etc.). The same applies to Fig. 2 (see below).

Minor issues:

Manuscript:

Code:

Reproducibility:

I tested the code on: MacBook Pro 13, Early 2015 OS X El Capitan Version 10.11.1 Processor: Intel Core i5 (2,7 GHz, 1 processor, 2 cores, L2 cache per core: 256 KB, L3 Cache: 3 MB) RAM: 16 GB Python: Version 3.6.1

-Here are the plots that I have obtained:

~Here, I annotated significant differences for Fig 2:~ ~- grid_l_dt_100us_mean_anno.pdf~ ~- grid_nl_dt_100us_mean_anno.pdf~

These are my main comments, and I will further review the code more thoroughly.

In conclusion, I am grateful for the work that the authors put into this replication study and acknowledge the willingness by the author of the original article to help with original code and clarifications.

Cheers, Dennis Goldschmidt

rougier commented 6 years ago

@degoldschmidt Thanks for the detailed review. Concerning images, you drag and drop them into the comment and it will appear in the once submitted. You can either edit your commend to add them or create a new comment below this one.

degoldschmidt commented 6 years ago

I actually rechecked the results from the original repository and have to apologize because the differences were because of my printed out version of the manuscript. I will re-edit the original comment on that issue, and again, my apologies for this mistake.

rougier commented 6 years ago

Forgot to mention: if you want the images to be displayed inline, they have to be bitmaps (e.g. png).

rcaze commented 6 years ago

@rougier @degoldschmidt @pietromarchesi Thank you for the thorough review. We will address them in full and send you a new manuscript version with detailed explanations of our changes by Friday March 9th. Best,

mstimberg commented 6 years ago

@rougier @degoldschmidt @pietromarchesi Thank you again for the constructive reviews. In response to your remarks and suggestions we have added additional simulations and analysis, but unfortunately we will need a few more days to fully include these changes in the manuscript and in our response to the reviewers. We will send you the new manuscript/code and the detailed response at the beginning of next week. Apologies for the delay.

mstimberg commented 6 years ago

We would like to again thank the reviewers for their valuable and constructive comments. We have updated the code and the manuscript, and additionally added a file Caze_2018_diff.pdf which shows the changes in the manuscript (changes in figures/tables/equations/code are not always well presented, but the document should give a good overview of which parts of the text have changed).

In preparing the revision, we noticed an error in our plotting code. The network firing rate plotted in Fig.1 (middle row) was incorrectly based on a bin size of 0.1ms, instead of the 1ms that was used in the original publication (and also stated in our figure caption). Therefore, the (non-synchronous) background activity appeared to be 10 times lower than it actually was.

Below we describe, when needed, how we changed the code and the manuscript to adress the reviewers' comments.

Reviewer 1 (@pietromarchesi)

There's a small typo in the code README, last section on parallelization reads 'multiple CPU cores a moder computer provide'.

We corrected the typo in the README file.

In Figure 2 (of the replication), I appreciate the switch from the epsilon+subscript axis labels used in the paper to plain text, which avoids having to read the caption. In line with that, I think using full text for inhibitory and excitatory (instead of Inh and ext) would be even clearer.

We modified the axis labels accordingly.

Apologies if I missed it, but I don't understand why the replication of Figure 3c is skipped. I understand that the crux of the matter is captured by a and b, but was there a particular reason why it was omitted (e.g. computation time)?

There was no particular reason for this choice and we have now added the simulation and the corresponding plot to our Figure 2.

I would encourage, in the concluding Discussion section, a brief recap of the differences found (detail in Figure 2, underestimation in Figure 3) before discussing the potential causes.

We modified the discussion accordingly (see L451 in the .tex file).

I haven't gone through the details of the derivation of the semi-analytic solution yet, but I think it would be useful to have a sentence or two specifying if/where the derivation presented differs from the original one.

We clarified this in the text (see L345 in the .tex file). The derivation is just provided as a more detailed description of the approach presented in the original article but is otherwise identical. There is a potential difference in the way the observed membrane potential distribution is used as an estimate of P(V), as the details (e.g. number of bins for the histogram, use of interpolation/smoothing, etc.) were not mentioned in the original article.

I don't fully agree with the the statement that the semi-analytical result underestimates the numerical one, if anything to me it looks shifted to the right (underestimates before the peak, overestimates after). I am slightly confused by how you obtain a numerical approximation of P(V) as opposed to how the original paper does it.

We have added a new paragraph to the text (see L338 in the .tex file) that introduces a correction to the plotted values, bringing the numerical results into closer agreement with the original study. Figure 3 has been updated accordingly and the previous version of the figure (without the correction) has been added as a supplementary figure (Figure 7). We have also added more discussion of the quantitative differences between our results and the original study (L444 in the .tex file).

Reviewer 2 (@degoldschmidt)

Major issue:

First, while many of the decisions made by the authors have been explained, such as the choice of Brian and certain parameter adjustments, I am missing an explanation for why the authors decided not to use the original analytical solution as well.

We now further justify our choice in the main text (see L339 in .tex file). We found that the analytical approach was not described in as much detail as the semi-analytical one and therefore focussed on the latter. If the reviewers think that including the analytical solution is important, then we can contact the original authors to have further details.

Furthermore, differences between this Figure (3) and the original that are described are too short. Instead of only replicating the figure and describing a "good match", I would like to see a more quantitative comparison (e.g., position of the maximum, intersections, etc.). The same applies to Fig. 2 (see below).

We have added some more quantitative comparison to the results in the original work for Figure 2 (See L423 in the .tex file) and Figure 3 (see L444 in the .tex file), as well as an extended discussion of the source of the differences via the additional simulations presented in the supplementary Figures 5 and 6.

Minor issues

We have corrected the typographical issues (spaces, commas, etc.) as suggested by the reviewer, as well as rewritten several sentences to make them more clear and to avoid parentheses. We have also aligned xticks/yticks with the values used in the original publication.

In lib.py, I am a bit puzzled about the way how the function "run_with_cache" is defined. Why can't the run function be decorated with @mem.cache as for the other functions?

Functions that are to be used with joblib's Parallel/delayed mechanism cannot use the @mem.cache decorator directly (this is a known limitation of joblib and has to do with the inability of Python's pickle mechanism to pickle two functions that have the same name). We have now added a more detailed comment about this to the first time in lib.py where this occurs (the definition of calc_group_evolution_cached) and refer back to this comment for the other functions such as run_with_cache.

Could the authors elaborate on the definition of V0 in line 81. It does not seem to be included in Table 1 or the main text.

V0 is the membrane potential displacement due to a constant input current. It is included in equation 1 and mentioned in the surrounding text, but its value was never mentioned. We now include it in Table 1.

The description for implementing the nonlinear networks could be rewritten to match closer the mathematical definitions. Start with describing the use of the clipping function to achieve the nonlinearity function, then explain that ve is used as a placeholder variable equivalent to x from the equation.

We rewrote the text according to the reviewer's recommendations (see L311 in the .tex file)

The C panel from the original study has not been replicated and further the differences could be described in more detail. I also see differences for large weights (both exc. and inh.), as well as qualitative differences for the linear network (no yellow line/areas). In B, the area of unstable activity before stimulation seems to be significantly larger.

The C panel has now been replicated and we include further details about the quantitative differences to the original study (See L423 in the .tex file).

Why did the authors not try the initial random spikes in transit?

Our simulations now include initial random spikes, although the details of their implementation was not clear from the text.

I got thrown a ton of FutureWarnings concerning the use of certain dtypes in Brian. I think this is related to the version/system I was using (although I used the requirements.txt to get the exact version numbers). I omitted the warnings by using the Warnings library to ignore any FutureWarnings

These warnings should only appear with numpy 1.14, which was released after the latest release of Brian 2. Please double check that you are using numpy 1.13.3 as stated in requirements.txt. That said, the warnings can be safely ignored and Brian 2 does work correctly with numpy 1.14.

pietromarchesi commented 6 years ago

@mstimberg Thank you for addressing the reviews. I have been out of office in the past few weeks, and I apologize for the radio silence. I aim to read through your responses in the coming week or so.

degoldschmidt commented 6 years ago

@mstimberg Thank you for the revised version. I am happy to see that the authors agreed with the suggestions.

We now further justify our choice in the main text (see L339 in .tex file). We found that the analytical approach was not described in as much detail as the semi-analytical one and therefore focussed on the latter. If the reviewers think that including the analytical solution is important, then we can contact the original authors to have further details.

The authors sufficiently explain the choice for omitting the analytical solution. It does not add any further to the replication as it was developed in a different paper.

We have added some more quantitative comparison to the results in the original work for Figure 2 (See L423 in the .tex file) and Figure 3 (see L444 in the .tex file), as well as an extended discussion of the source of the differences via the additional simulations presented in the supplementary Figures 5 and 6.

Very nice. I especially like that the authors after identifying that the differences between the semi-analytical and numerical solution, tried to find a corrected set of parameters that result in a better quantitative fit.

We have corrected the typographical issues (spaces, commas, etc.) as suggested by the reviewer, as well as rewritten several sentences to make them more clear and to avoid parentheses. We have also aligned xticks/yticks with the values used in the original publication.

Good job, I only have a minor issue with Eq. 2, where the commas should be each placed after the case and after the case clause. Furthermore, there are a few inconsistencies in terms of the use of British vs. American spelling: the authors use 'behaviour' and 'modelled', but also 'synchronized', 'summarized' and 'color'.

Functions that are to be used with joblib's Parallel/delayed mechanism cannot use the @mem.cache decorator directly (this is a known limitation of joblib and has to do with the inability of Python's pickle mechanism to pickle two functions that have the same name). We have now added a more detailed comment about this to the first time in lib.py where this occurs (the definition of calc_group_evolution_cached) and refer back to this comment for the other functions such as run_with_cache.

This fully answers my question, thank you.

V0 is the membrane potential displacement due to a constant input current. It is included in equation 1 and mentioned in the surrounding text, but its value was never mentioned. We now include it in Table 1.

We rewrote the text according to the reviewer's recommendations (see L311 in the .tex file)

The C panel has now been replicated and we include further details about the quantitative differences to the original study (See L423 in the .tex file).

Our simulations now include initial random spikes, although the details of their implementation was not clear from the text.

Thank you for all these changes and addition, which are helpful for further justifying the successful replication apart from the discussed differences. Especially, the discussion is now well-structured and logical. Minor point:

"The differences we see are most likely due to our use of a clock-based instead of event-driven simulations, as it forces all activity to a temporal grid of finite precision."

These warnings should only appear with numpy 1.14, which was released after the latest release of Brian 2. Please double check that you are using numpy 1.13.3 as stated in requirements.txt. That said, the warnings can be safely ignored and Brian 2 does work correctly with numpy 1.14.

This was indeed the case. My PC probably didn't want to downgrade.

rougier commented 6 years ago

@pietromarchesi Does that mean you accept the submission? @degoldschmidt Did you have time to look at the revised submission?

degoldschmidt commented 6 years ago

I guess the @ mentions were meant the other way around. I would accept the submission in the revised form.

rougier commented 6 years ago

Yes, sorry for the mixup

pietromarchesi commented 6 years ago

I would like to again thanks the authors for addressing the reviews.

We have added a new paragraph to the text (see L338 in the .tex file) that introduces a correction to the plotted values, bringing the numerical results into closer agreement with the original study. Figure 3 has been updated accordingly and the previous version of the figure (without the correction) has been added as a supplementary figure (Figure 7). We have also added more discussion of the quantitative differences between our results and the original study (L444 in the .tex file).

Great!

While reading the updated article, I spotted two typos:

I am overall very happy with the state of the submission, and have just one final comment: line 423 (.tex) reads:

For linear networks, we do observe almost no simulations classified in this category, while the original article has a thin line of such classifications between the stable regime without propagation (green) and the regime where simulations are unstable even without stimulation (red).

I looked back at the original Fig. 3 and I struggle to see the aforementioned thin yellow line. Can you explain where you see that?

mstimberg commented 6 years ago

@degoldschmidt, @pietromarchesi : many thanks again for your comments, we will address the mentioned typos and inconsistencies between British and American spelling in the next few days.

I looked back at the original Fig. 3 and I struggle to see the aforementioned thin yellow line. Can you explain where you see that?

@pietromarchesi Many thanks for making us aware of this, it turns out that the thin yellow line only exist in my printed version of the paper and therefore seems to be an artifact... Apologies, we will remove the misleading sentence.

mstimberg commented 6 years ago

Dear reviewers, we have updated the text, fixed the typos and switched to a consistent use of British spelling. We have also removed the outdated "diff file" from the repository.

@degoldschmidt I'm not 100% sure that I correctly understood your proposed improvement of Eq. 2, please double-check.

pietromarchesi commented 6 years ago

@mstimberg Thank you for the last commits. @rougier I accept the submission.

degoldschmidt commented 6 years ago

@mstimberg this is exactly how I meant it, thank you for revising.

@pietromarchesi @mstimberg concerning the thin yellow line, I had the same problem with my printout version.

rougier commented 6 years ago

@degoldschmidt Do you accept the submission then?

degoldschmidt commented 6 years ago

@rougier I accept the submission. Thank you all for the great work!

rougier commented 6 years ago

Thanks @degoldschmidt and @pietromarchesi for you reviews. @rcaze Congratulations, your submission has been accepted and should be published soon (hopefully).

rougier commented 6 years ago

@caze @mstimberg @benoit-girard Do you have any markdown of your article? I cannot find it in the repo

rougier commented 6 years ago

@caze @mstimberg @benoit-girard 🛎

rougier commented 6 years ago

Published as DOI.

It should appear soon on http://rescience.github.io/read/