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 Request: Caze Stimberg Girard #45

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@rcaze

rcaze commented Jan 24, 2018

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

  • Article has been fully replicated
  • Article has been partially replicated
  • Article has not been replicated

Potential reviewers


EDITOR

  • Editor acknowledgment (@rougier January 26, 2018)
  • Reviewer 1 (@pietromarchesi January 26, 2018)
  • Reviewer 2 (@degoldschmidt January 26, 2018)
  • Review 1 decision [accept] (April 17, 2018)
  • Review 2 decision [accept] (April 23, 2018)
  • Editor decision [accept] (April 23, 2018)
@pietromarchesi

This comment has been minimized.

pietromarchesi commented Jan 25, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Jan 26, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Jan 26, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Jan 26, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Jan 26, 2018

@degoldschmidt Can you review this submission?

@pietromarchesi

This comment has been minimized.

pietromarchesi commented Jan 26, 2018

@rougier Yes, sure!

@degoldschmidt

This comment has been minimized.

degoldschmidt commented Jan 26, 2018

@rougier Yes, of course!

@rougier rougier added the 02 - Review label Jan 26, 2018

@rougier

This comment has been minimized.

Member

rougier commented Jan 26, 2018

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

@pietromarchesi

This comment has been minimized.

pietromarchesi commented Feb 2, 2018

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

This comment has been minimized.

mstimberg commented Feb 2, 2018

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.

@rougier rougier changed the title from Caze stimberg girard to Review Request: Caze Stimberg Girard Feb 5, 2018

@rcaze

This comment has been minimized.

rcaze commented Feb 8, 2018

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

This comment has been minimized.

Member

rougier commented Feb 9, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Feb 16, 2018

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

@rcaze

This comment has been minimized.

rcaze commented Feb 26, 2018

@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

This comment has been minimized.

mstimberg commented Mar 9, 2018

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

Update manuscript and code for the new revision
Co-authored-by: Romain Caze <romain.caze@gmail.com>
@mstimberg

This comment has been minimized.

mstimberg commented Mar 13, 2018

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

This comment has been minimized.

pietromarchesi commented Apr 5, 2018

@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

This comment has been minimized.

degoldschmidt commented Apr 11, 2018

@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

This comment has been minimized.

Member

rougier commented Apr 15, 2018

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

@degoldschmidt

This comment has been minimized.

degoldschmidt commented Apr 15, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Apr 16, 2018

Yes, sorry for the mixup

@pietromarchesi

This comment has been minimized.

pietromarchesi commented Apr 17, 2018

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:

  • Equation 2, second line: there's one comma too many.
  • L. 306 of the .tex file, which reads 'We used a the predefined \verb|clip| function to implemented non-linear coupling into a network', contains two mistakes.

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

This comment has been minimized.

mstimberg commented Apr 17, 2018

@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

This comment has been minimized.

mstimberg commented Apr 17, 2018

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

This comment has been minimized.

pietromarchesi commented Apr 17, 2018

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

@degoldschmidt

This comment has been minimized.

degoldschmidt commented Apr 19, 2018

@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

This comment has been minimized.

Member

rougier commented Apr 23, 2018

@degoldschmidt Do you accept the submission then?

@degoldschmidt

This comment has been minimized.

degoldschmidt commented Apr 23, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented Apr 23, 2018

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

@ReScience ReScience locked as resolved and limited conversation to collaborators Apr 26, 2018

@rougier rougier referenced this pull request Apr 26, 2018

Open

Article number request #48

@rougier

This comment has been minimized.

Member

rougier commented Apr 26, 2018

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

@rougier

This comment has been minimized.

Member

rougier commented May 2, 2018

@ReScience ReScience unlocked this conversation May 2, 2018

@rougier

This comment has been minimized.

Member

rougier commented May 14, 2018

Published as DOI.

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

@ReScience ReScience locked and limited conversation to collaborators May 14, 2018

@rougier rougier closed this May 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.