Review Request: Metzner #30

Closed
wants to merge 58 commits into
from

Conversation

Projects
None yet
6 participants
@ChristophMetzner

ChristophMetzner commented Apr 19, 2017

Christoph Metzner

Dear @ReScience/editors,

I request a review for the following replication:

AUTHOR

Original article

Title: Modeling GABA alterations in schizophrenia: a link between impaired inhibition and altered gamma and beta range auditory entrainment
Author(s): Vierling-Claassen, D., Siekmeier, P., Stufflebeam, S., & Kopell, N.
Journal (or Conference): Journal of Neurophysiology
Year: 2008
DOI: 10.1152/jn.00870.2007
PDF: http://jn.physiology.org/content/99/5/2656

Replication

Author(s): Christoph Metzner
Repository: https://github.com/ChristophMetzner/ReScience-submission/tree/Metzner-2017
PDF: https://github.com/ChristophMetzner/ReScience-submission/tree/Metzner-2017/article/Metzner-2017.pdf
Keywords: Neuroscience, Gamma and Beta Oscillations, Auditory Entrainment, Schizophrenia, Python
Language: Python
Domain: Computational Neuroscience

Results

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

Potential reviewers


EDITOR

  • Editor acknowledgment (@oliviaguest 24 April 2017)
  • Reviewer 1 (@aaronshifman 25 April 2017)
  • Reviewer 2 (@pietromarchesi 25 April 2017)
  • Review 1 decision [accept]
  • Review 2 decision [accept]
  • Editor decision [accept]
@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 19, 2017

Member

@ChristophMetzner Thanks for your submission, we'll assign an editor soon. Do you have a link to an open-access version of the original article ?

Member

rougier commented Apr 19, 2017

@ChristophMetzner Thanks for your submission, we'll assign an editor soon. Do you have a link to an open-access version of the original article ?

@rougier rougier changed the title from Metzner 2017 to Review Request: Metzner 2017 Apr 19, 2017

@rougier rougier changed the title from Review Request: Metzner 2017 to Review Request: Metzner Apr 19, 2017

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 19, 2017

Member

@otizonaizit Could you handle this submission ?

Member

rougier commented Apr 19, 2017

@otizonaizit Could you handle this submission ?

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Apr 20, 2017

@rougier Thank you. Here is a link to the original article http://jn.physiology.org/content/99/5/2656

@rougier Thank you. Here is a link to the original article http://jn.physiology.org/content/99/5/2656

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Apr 21, 2017

Member

@rougier : Hi, Nicolas! I won't be able to take care of this in a timely manner for the next month or two, so it's probably better to look for a different editor. If you don't find anyone suitable, please come back to me!

Member

otizonaizit commented Apr 21, 2017

@rougier : Hi, Nicolas! I won't be able to take care of this in a timely manner for the next month or two, so it's probably better to look for a different editor. If you don't find anyone suitable, please come back to me!

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 21, 2017

Member

@otizonaizit Thanks for the quick answer. I'll look for another editor.

Member

rougier commented Apr 21, 2017

@otizonaizit Thanks for the quick answer. I'll look for another editor.

@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman Apr 21, 2017

I might be early: but I'd volunteer as reviewer.

I might be early: but I'd volunteer as reviewer.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 24, 2017

Member

@oliviaguest Can you handle this submission ?

Member

rougier commented Apr 24, 2017

@oliviaguest Can you handle this submission ?

@oliviaguest

This comment has been minimized.

Show comment
Hide comment

Sure.

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest Apr 25, 2017

@rougier whose responsibility is it to re-compile the pdf to make sure the internal references show up without question marks and without just outputting the labels, etc.?

oliviaguest commented Apr 25, 2017

@rougier whose responsibility is it to re-compile the pdf to make sure the internal references show up without question marks and without just outputting the labels, etc.?

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 25, 2017

Member

@oliviaguest The one who asks 😄 More seriously, you can ask author(s) to try to correct for the internal references. Because they know their manuscript much better than anyone else.
Most probably this might be due to trailing whitespaces or a pandoc version mismatch. But if not, we (edtiros) will have to give a closer look.

Member

rougier commented Apr 25, 2017

@oliviaguest The one who asks 😄 More seriously, you can ask author(s) to try to correct for the internal references. Because they know their manuscript much better than anyone else.
Most probably this might be due to trailing whitespaces or a pandoc version mismatch. But if not, we (edtiros) will have to give a closer look.

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest Apr 25, 2017

@ChristophMetzner could you take a look at the above issue regarding internal references, please?

@ChristophMetzner could you take a look at the above issue regarding internal references, please?

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest Apr 25, 2017

@pietromarchesi got back to me super quickly — so we have both reviewers ready to go! 👍

@pietromarchesi got back to me super quickly — so we have both reviewers ready to go! 👍

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 25, 2017

Member

@oliviaguest @aaronshifman @pietromarchesi You've been granted a new ⭐️ on the board page (it's a link to the review).

@pietromarchesi Can you give me your ORCID number?

Member

rougier commented Apr 25, 2017

@oliviaguest @aaronshifman @pietromarchesi You've been granted a new ⭐️ on the board page (it's a link to the review).

@pietromarchesi Can you give me your ORCID number?

@pietromarchesi

This comment has been minimized.

Show comment
Hide comment
@pietromarchesi

pietromarchesi Apr 26, 2017

@rougier My ORCID is 0000-0001-5955-6909.

@rougier My ORCID is 0000-0001-5955-6909.

@pietromarchesi

This comment has been minimized.

Show comment
Hide comment
@pietromarchesi

pietromarchesi Apr 26, 2017

@oliviaguest Can we begin a conversation with the authors and discuss fixes as we encounter problems/imperfections, or should we wait and post a more thorough revision? I have already noticed a few small things that the author could start to fix as I work my way through the rest of the code.

Also, I cannot seem to clone the forked repo. It clones the original ReScience-submission repository, not the forked one (downloading the .zip on the other hand works fine). If anybody knows how to fix this, it would be helpful.

pietromarchesi commented Apr 26, 2017

@oliviaguest Can we begin a conversation with the authors and discuss fixes as we encounter problems/imperfections, or should we wait and post a more thorough revision? I have already noticed a few small things that the author could start to fix as I work my way through the rest of the code.

Also, I cannot seem to clone the forked repo. It clones the original ReScience-submission repository, not the forked one (downloading the .zip on the other hand works fine). If anybody knows how to fix this, it would be helpful.

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest Apr 26, 2017

@rougier will hopefully know the answers to both your questions @pietromarchesi!

@rougier will hopefully know the answers to both your questions @pietromarchesi!

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Apr 26, 2017

@oliviaguest I will look at the references asap.

@oliviaguest I will look at the references asap.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Apr 26, 2017

Member

@pietromarchesi you have to switch to the author branch and pull again.

Member

rougier commented Apr 26, 2017

@pietromarchesi you have to switch to the author branch and pull again.

@pietromarchesi

This comment has been minimized.

Show comment
Hide comment
@pietromarchesi

pietromarchesi Apr 28, 2017

(REVIEWER 1) I looked through the submission and noted down some comments.

Preliminaries

The readme instructs to run run_example.py, whereas the file is example_run.py.

The method rasterPlot of the class simpleModel in simple_model_class.py takes an additional argument name which is not provided on lines 55 and 56 of example_run.py.

On line 55 of run_simulations.py, the path of the Numpy seed is incorrect: it needs to be changed to ../data/Seeds.npy. In the line below, the name of the directory has to be changed from '../data_run'
to ../data.

In run_simulations.py, I noticed that the files which contain the average over trials (i.e. sims_ctrl_avg_20Hz.npy) which are later called in plot_figures.py, are never saved. I added that
manually on line 84 of run_simulations.py:

	filename_ctrl_avg = directory+'/sims_ctrl_avg_'+str(int(drive_frequency))+'Hz'
   filename_schiz_avg = directory+'/sims_schiz_avg_'+str(int(drive_frequency))+'Hz'

   np.save(filename_ctrl_avg, meg_ctrl_avg)
   np.save(filename_schiz_avg, meg_schiz_avg)

And then changed the path to the files in plot_figures.py, where they are loaded around line 51. I did not make a specific folder for these average-over-trial results, but perhaps it might be cleaner to do so.

Note that in plot_figures.py, the /data_run repository has to be renamed to /data wherever it appears.

Finally, I think the file analysis2.py is an older and not commented version of analysis.py which should probably be removed.

I now see that at least some of my previous comments are already corrected in the notebooks, but the changes still need to be transferred to the .py files.

Code

At the expense of efficiency, the code is very readable, scoring points in article 7 of the zen of Python.

One minor flaw that I happened to notice is that lines 137 and 150 in simple_model_class.py are indented one too many times. This results in the list of spike times being saved every time and not just at the end of the while loop when it's complete.

Not to be a PEP8 nazi (nobody likes those) but the very long equations in simpleModel.run could perhaps be split over two lines, as they are basically double the recommended line length.

Perhaps the comments on lines 167 and 171 in simple_model_class.py, which state that the following lines 'seem awfully complicated' could be replaced or integrated with a line that refers to the
corresponding equation in the article.

Replication

Overall, the main findings are indeed replicated, however, as recognized by the author, some
of them are slightly less prominent than in the original paper. In this regard it is not clear to me what the author @ChristophMetzner means when he indicates that this might be due to a difference in noise: where could this difference arise? Are you referring to just stochastic effects or to something else entirely? It might be good to make this a bit clearer in the article as well.

If my understanding is correct, the emergence of a 20 Hz component is due to the fact that longer decay of the IPSC's hinders the 40Hz entrainment and you get responses to alternate inputs from the drive. In the absence of noise, your 40Hz-drive schizophrenic network will respond only with 20Hz, and increasing noise brings in the 40Hz component, because some cells still have enough input to fire in spite of the prolonged inhibition. All of this is to say that perhaps scaling the noise a bit lower could give you more of the 20Hz and 40Hz, making the result of the paper more marked. Have you tried anything like that?

Other

The labels in the plots of the article are a bit too small to be readable.

pietromarchesi commented Apr 28, 2017

(REVIEWER 1) I looked through the submission and noted down some comments.

Preliminaries

The readme instructs to run run_example.py, whereas the file is example_run.py.

The method rasterPlot of the class simpleModel in simple_model_class.py takes an additional argument name which is not provided on lines 55 and 56 of example_run.py.

On line 55 of run_simulations.py, the path of the Numpy seed is incorrect: it needs to be changed to ../data/Seeds.npy. In the line below, the name of the directory has to be changed from '../data_run'
to ../data.

In run_simulations.py, I noticed that the files which contain the average over trials (i.e. sims_ctrl_avg_20Hz.npy) which are later called in plot_figures.py, are never saved. I added that
manually on line 84 of run_simulations.py:

	filename_ctrl_avg = directory+'/sims_ctrl_avg_'+str(int(drive_frequency))+'Hz'
   filename_schiz_avg = directory+'/sims_schiz_avg_'+str(int(drive_frequency))+'Hz'

   np.save(filename_ctrl_avg, meg_ctrl_avg)
   np.save(filename_schiz_avg, meg_schiz_avg)

And then changed the path to the files in plot_figures.py, where they are loaded around line 51. I did not make a specific folder for these average-over-trial results, but perhaps it might be cleaner to do so.

Note that in plot_figures.py, the /data_run repository has to be renamed to /data wherever it appears.

Finally, I think the file analysis2.py is an older and not commented version of analysis.py which should probably be removed.

I now see that at least some of my previous comments are already corrected in the notebooks, but the changes still need to be transferred to the .py files.

Code

At the expense of efficiency, the code is very readable, scoring points in article 7 of the zen of Python.

One minor flaw that I happened to notice is that lines 137 and 150 in simple_model_class.py are indented one too many times. This results in the list of spike times being saved every time and not just at the end of the while loop when it's complete.

Not to be a PEP8 nazi (nobody likes those) but the very long equations in simpleModel.run could perhaps be split over two lines, as they are basically double the recommended line length.

Perhaps the comments on lines 167 and 171 in simple_model_class.py, which state that the following lines 'seem awfully complicated' could be replaced or integrated with a line that refers to the
corresponding equation in the article.

Replication

Overall, the main findings are indeed replicated, however, as recognized by the author, some
of them are slightly less prominent than in the original paper. In this regard it is not clear to me what the author @ChristophMetzner means when he indicates that this might be due to a difference in noise: where could this difference arise? Are you referring to just stochastic effects or to something else entirely? It might be good to make this a bit clearer in the article as well.

If my understanding is correct, the emergence of a 20 Hz component is due to the fact that longer decay of the IPSC's hinders the 40Hz entrainment and you get responses to alternate inputs from the drive. In the absence of noise, your 40Hz-drive schizophrenic network will respond only with 20Hz, and increasing noise brings in the 40Hz component, because some cells still have enough input to fire in spite of the prolonged inhibition. All of this is to say that perhaps scaling the noise a bit lower could give you more of the 20Hz and 40Hz, making the result of the paper more marked. Have you tried anything like that?

Other

The labels in the plots of the article are a bit too small to be readable.

@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman May 1, 2017

Code

The code was fairly well commented, however in some places it could do with some more comments. For example generating the spike trains (simple_model_class: line 130-140).

I'm not sure I understand _getSingleSpikeTimes - what being iterated over? Is that time and state?

I'm also unclear as to what is going on in calculatePSD. If I understand correctly meg is a time series and your slicing starting at an index that is 20% of the sampling frequency? I think this could use more comments

As @ChristophMetzner mentioned he was going for a readable over efficiency model for the code. However there are some places where the explicit nature of the code detracts from readability.

For example (line 128) just scales background_rate by 1000
rate_parameter = 1000*(1.0/self.background_rate)
rate_parameter = 1.0/rate_parameter

As the other reviewer mentioned: the exceptionally long expressions should be broken up in some way for readability.

Out of the box I was unable to run the code as written for several reasons

  1. example_run was renamed run_example
  2. data_run was renamed data
  3. The code is not python 3 friendly (print). I don't know what the policy is on compatibility with python 3 @oliviaguest any idea?

I made brief changes to allow the code to run and it did so. However plot_figures could not run as the naming convention in the data folder had changed. This needs to be addressed as I have not yet confirmed the plots match the article.

Lastly not at all important the comments for simpleModel have a variable number of tabs

Article

As a comment the cross referencing seemed to have failed. I recall a similar issue and I put in explicit latex \ref and \label tags.

In general I felt the article was fairly sparse and could do with more text and commentary everywhere. Specifically in the introduction: a more general introduction was needed, what are the results, why are they important?

For the methods tables 1 and 2 should really be written into the text - they addresses several questions that arise while reading the text and they don't lend themselves to tabular form. Also the python versions are ?.?.? I don't know if that a cross-ref error or a place holder. Also the OS this was run on should be stated.

Lastly a sentence on the numerical methods is needed - nothing much just forward Euler and a statement (if true) that the results are unaffected by a smaller time step.

There are a few things in the results that need to be addressed.
Firstly the summary of the main features should be presented in a clearer fashion... maybe this could be in a table?

Also in figure 2 (their figure 5) the frequency ranges are different. For example in the original 30Hz (controll) has a peak ~70 Hz but your implementation stops at 50Hz.

In the original paper power is in nA m^2 Hz^-1 can you scale your power measure to match? - you mention a scaling factor - have you tried to contact the original authors?

Also to match the original paper the raster plots should be flipped so that I cells are on the bottom.

All axis and tick labels need to be much bigger.

As for your comment about about being less pronounced, you proposed that it arises from the fact that there is a random process. What happens if you change the seed - does the effect size change? In general this implementation does seem to replicate the original findings however, more work as to the nature of the discrepancy and its origin is required.

Code

The code was fairly well commented, however in some places it could do with some more comments. For example generating the spike trains (simple_model_class: line 130-140).

I'm not sure I understand _getSingleSpikeTimes - what being iterated over? Is that time and state?

I'm also unclear as to what is going on in calculatePSD. If I understand correctly meg is a time series and your slicing starting at an index that is 20% of the sampling frequency? I think this could use more comments

As @ChristophMetzner mentioned he was going for a readable over efficiency model for the code. However there are some places where the explicit nature of the code detracts from readability.

For example (line 128) just scales background_rate by 1000
rate_parameter = 1000*(1.0/self.background_rate)
rate_parameter = 1.0/rate_parameter

As the other reviewer mentioned: the exceptionally long expressions should be broken up in some way for readability.

Out of the box I was unable to run the code as written for several reasons

  1. example_run was renamed run_example
  2. data_run was renamed data
  3. The code is not python 3 friendly (print). I don't know what the policy is on compatibility with python 3 @oliviaguest any idea?

I made brief changes to allow the code to run and it did so. However plot_figures could not run as the naming convention in the data folder had changed. This needs to be addressed as I have not yet confirmed the plots match the article.

Lastly not at all important the comments for simpleModel have a variable number of tabs

Article

As a comment the cross referencing seemed to have failed. I recall a similar issue and I put in explicit latex \ref and \label tags.

In general I felt the article was fairly sparse and could do with more text and commentary everywhere. Specifically in the introduction: a more general introduction was needed, what are the results, why are they important?

For the methods tables 1 and 2 should really be written into the text - they addresses several questions that arise while reading the text and they don't lend themselves to tabular form. Also the python versions are ?.?.? I don't know if that a cross-ref error or a place holder. Also the OS this was run on should be stated.

Lastly a sentence on the numerical methods is needed - nothing much just forward Euler and a statement (if true) that the results are unaffected by a smaller time step.

There are a few things in the results that need to be addressed.
Firstly the summary of the main features should be presented in a clearer fashion... maybe this could be in a table?

Also in figure 2 (their figure 5) the frequency ranges are different. For example in the original 30Hz (controll) has a peak ~70 Hz but your implementation stops at 50Hz.

In the original paper power is in nA m^2 Hz^-1 can you scale your power measure to match? - you mention a scaling factor - have you tried to contact the original authors?

Also to match the original paper the raster plots should be flipped so that I cells are on the bottom.

All axis and tick labels need to be much bigger.

As for your comment about about being less pronounced, you proposed that it arises from the fact that there is a random process. What happens if you change the seed - does the effect size change? In general this implementation does seem to replicate the original findings however, more work as to the nature of the discrepancy and its origin is required.

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest May 2, 2017

@rougier is there any policy regarding python versions?

oliviaguest commented May 2, 2017

@rougier is there any policy regarding python versions?

@pietromarchesi

This comment has been minimized.

Show comment
Hide comment
@pietromarchesi

pietromarchesi May 24, 2017

@ChristophMetzner, I agree that "Regarding the 'mistake' in the table, I think that it is not a mistake. The subharmonic component emerges at 40Hz drive and, therefore, is at 20Hz in the power spectrum.", but it used to say that it was at 40Hz before your last few updates. It is all good now.

Perhaps something will have to done about the formatting, there's many pages which now contain only one plot and are almost empty.

pietromarchesi commented May 24, 2017

@ChristophMetzner, I agree that "Regarding the 'mistake' in the table, I think that it is not a mistake. The subharmonic component emerges at 40Hz drive and, therefore, is at 20Hz in the power spectrum.", but it used to say that it was at 40Hz before your last few updates. It is all good now.

Perhaps something will have to done about the formatting, there's many pages which now contain only one plot and are almost empty.

@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman Jun 4, 2017

Article

The article is much improved and cleaner compared to the previous version, however I still have a few issues, (in the order they appear)

  • "are important for two reasons: First" - either colon and lowercase or two sentences

  • "wide range of deficits in schizophrenic patients Second" - missing period

  • "The neuron is described by a single variable \theta, which can be regarded as the membrane voltage..." - I think instead of treating \theta as membrane potential you could call it neuron state or oscillation phase.

  • \frac{d\theta}{dt}

    • you index every other equation so \theta and S should have an index
  • \alpha_j = \pm 1... \alpha_j isn't both it's either or, this should be clearer and represented in text or a piecewise function (but text would be clearer).
    -you never define \eta or epsc (while it's a common acronym it should still be defined once).

  • Should table captions be above or below the table, this seems like an editorial issue

  • What do you mean by topology none (table 1), why is topology separate from connectivity.
    -eta is missing from table 2

  • you mention the scaling again, but you cannot verify the original code - have you tried to contact the original authors?

  • crossref issue at fig:Vierling7

  • I appreciate what you're trying to do with the exploration of discrepancies part - I feel like it needs a bit more explanation and clarification. Are you able to estimate how far off the reported parameters and the parameters you would "need" to match the presented results are?

Code

The code as submitted does run with the exception of a typo (first comment)
In general the code is better commented and presents better, however there are a few more things

  • Typo: line 23 of analysis.py there is no colon at the end of the function declaration
  • The comment about drive current (line 141) should be in the text
  • There is a large mix of tabs and spaces forming the indents, while Python 2 will still run this, it's not hard to change (a single auto-format in pycharm will correct it), and I believe after that it becomes Python 3 compatible
  • Variables are duplicated it appears (for example n_ex is defined as 20 in example_run as well as the default in simple_model_class.py). If it's constant leaving it as a default is fine, but remove the redundant declaration.
  • You have some unused variables ex. line 354 of simple_model_class.py
  • Many lines are still far too long

In general while observing pep8 to the nth degree may not be necessary certain things like max line lengths are really important for readability.

Figures

  • There is a lot of duplicated code in both plotting scripts, and this might be my personal preference bleeding in but I would prefer to have common plotting code as functions with parameters this (a) makes it cleaner and (b) makes it so that changes are far easier because I do have some requests for those
  • Figure 1 looks really good the only issue I have is that the matrix labels overshadow the axis labels by way too much and the axis labels are still too small. What I think would work is to have the two sets of labels the same size and leave the matrix labels bold.
  • the same thing applies for the tick labels, you could make fewer and make them a bit larger
  • I have the same comment for the rest of the figures, shrink the matrix labels and make the axis labels larger.
  • For figures 3-6 if you cut them by a bit (maybe 25%) you might be able to fit more than 1 per page and the paper would look far less sparse.

Article

The article is much improved and cleaner compared to the previous version, however I still have a few issues, (in the order they appear)

  • "are important for two reasons: First" - either colon and lowercase or two sentences

  • "wide range of deficits in schizophrenic patients Second" - missing period

  • "The neuron is described by a single variable \theta, which can be regarded as the membrane voltage..." - I think instead of treating \theta as membrane potential you could call it neuron state or oscillation phase.

  • \frac{d\theta}{dt}

    • you index every other equation so \theta and S should have an index
  • \alpha_j = \pm 1... \alpha_j isn't both it's either or, this should be clearer and represented in text or a piecewise function (but text would be clearer).
    -you never define \eta or epsc (while it's a common acronym it should still be defined once).

  • Should table captions be above or below the table, this seems like an editorial issue

  • What do you mean by topology none (table 1), why is topology separate from connectivity.
    -eta is missing from table 2

  • you mention the scaling again, but you cannot verify the original code - have you tried to contact the original authors?

  • crossref issue at fig:Vierling7

  • I appreciate what you're trying to do with the exploration of discrepancies part - I feel like it needs a bit more explanation and clarification. Are you able to estimate how far off the reported parameters and the parameters you would "need" to match the presented results are?

Code

The code as submitted does run with the exception of a typo (first comment)
In general the code is better commented and presents better, however there are a few more things

  • Typo: line 23 of analysis.py there is no colon at the end of the function declaration
  • The comment about drive current (line 141) should be in the text
  • There is a large mix of tabs and spaces forming the indents, while Python 2 will still run this, it's not hard to change (a single auto-format in pycharm will correct it), and I believe after that it becomes Python 3 compatible
  • Variables are duplicated it appears (for example n_ex is defined as 20 in example_run as well as the default in simple_model_class.py). If it's constant leaving it as a default is fine, but remove the redundant declaration.
  • You have some unused variables ex. line 354 of simple_model_class.py
  • Many lines are still far too long

In general while observing pep8 to the nth degree may not be necessary certain things like max line lengths are really important for readability.

Figures

  • There is a lot of duplicated code in both plotting scripts, and this might be my personal preference bleeding in but I would prefer to have common plotting code as functions with parameters this (a) makes it cleaner and (b) makes it so that changes are far easier because I do have some requests for those
  • Figure 1 looks really good the only issue I have is that the matrix labels overshadow the axis labels by way too much and the axis labels are still too small. What I think would work is to have the two sets of labels the same size and leave the matrix labels bold.
  • the same thing applies for the tick labels, you could make fewer and make them a bit larger
  • I have the same comment for the rest of the figures, shrink the matrix labels and make the axis labels larger.
  • For figures 3-6 if you cut them by a bit (maybe 25%) you might be able to fit more than 1 per page and the paper would look far less sparse.
@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman Jun 22, 2017

Thank you @ChristophMetzner, I'll take a look this weekend.

Thank you @ChristophMetzner, I'll take a look this weekend.

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Jun 23, 2017

@aaronshifman I have not yet addressed the Figures, still working on that.

@aaronshifman I have not yet addressed the Figures, still working on that.

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Jul 4, 2017

@aaronshifman I have just updated the plotting scripts and notebooks.

@aaronshifman I have just updated the plotting scripts and notebooks.

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Jul 4, 2017

@aaronshifman Regarding your other comments:

  • "What do you mean by topology none (table 1), why is topology separate from connectivity."
    By topology I simply mean the arrangement of cells, i.e. are they arranged in a 2D or 3D space having coordinates. While this would be better called geometry, many articles use the word topology (also reflected in the names some simulators use, e.g. NEST has a topology module which arranges cells in 2D or 3D space). In this network there is no need for cells being arranged in space, so there is no 'topology'. I guess the difference to connectivity is also apparent now.
  • "you mention the scaling again, but you cannot verify the original code - have you tried to contact the original authors?"
    I had contacted the authors in the beginning, about sharing their code in general, without getting aresponse. Therefore, I did not contact them again asking about the scaling.
  • " I appreciate what you're trying to do with the exploration of discrepancies part - I feel like it needs a bit more explanation and clarification. Are you able to estimate how far off the reported parameters and the parameters you would "need" to match the presented results are?"
    Unfortunately, I don't see a way to estimate how far off I am, given that we have this highly nonlinear system. I also see various other ways by which the discrepancies could be explained. For example, a 'small' change in the weight from I to E cells (an increase) would probably lead to a more pronounced 20Hz component for 40Hz drive in the schizophrenia network (since the total inhibition would be increased, and therefore, more E cells would be suppressed eevry other cycle). However, I think that this would again reduce the 40Hz component in response to 20Hz drive in the control network, similar to the noise exploration. I can explore this further, however, I am not sure what we will gain from that, since there are more possible ways than the one described above. A complete and thorough search of the parameter space, I feel, is a little bit beyond the scope of this replication, since the main features are being reproduced and only the effect being slightly reduced. Let me know what you think. @pietromarchesi What do you think?

@aaronshifman Regarding your other comments:

  • "What do you mean by topology none (table 1), why is topology separate from connectivity."
    By topology I simply mean the arrangement of cells, i.e. are they arranged in a 2D or 3D space having coordinates. While this would be better called geometry, many articles use the word topology (also reflected in the names some simulators use, e.g. NEST has a topology module which arranges cells in 2D or 3D space). In this network there is no need for cells being arranged in space, so there is no 'topology'. I guess the difference to connectivity is also apparent now.
  • "you mention the scaling again, but you cannot verify the original code - have you tried to contact the original authors?"
    I had contacted the authors in the beginning, about sharing their code in general, without getting aresponse. Therefore, I did not contact them again asking about the scaling.
  • " I appreciate what you're trying to do with the exploration of discrepancies part - I feel like it needs a bit more explanation and clarification. Are you able to estimate how far off the reported parameters and the parameters you would "need" to match the presented results are?"
    Unfortunately, I don't see a way to estimate how far off I am, given that we have this highly nonlinear system. I also see various other ways by which the discrepancies could be explained. For example, a 'small' change in the weight from I to E cells (an increase) would probably lead to a more pronounced 20Hz component for 40Hz drive in the schizophrenia network (since the total inhibition would be increased, and therefore, more E cells would be suppressed eevry other cycle). However, I think that this would again reduce the 40Hz component in response to 20Hz drive in the control network, similar to the noise exploration. I can explore this further, however, I am not sure what we will gain from that, since there are more possible ways than the one described above. A complete and thorough search of the parameter space, I feel, is a little bit beyond the scope of this replication, since the main features are being reproduced and only the effect being slightly reduced. Let me know what you think. @pietromarchesi What do you think?
@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Jul 4, 2017

@aaronshifman @pietromarchesi Now there are most of the time two figures per page (except for Figure 1 and Figures 7 and 8). Should I make 7 and 8 smaller so that they fit on one page?

@aaronshifman @pietromarchesi Now there are most of the time two figures per page (except for Figure 1 and Figures 7 and 8). Should I make 7 and 8 smaller so that they fit on one page?

@pietromarchesi

This comment has been minimized.

Show comment
Hide comment
@pietromarchesi

pietromarchesi Jul 19, 2017

@ChristophMetzner Thank you for your last commits and for your comments. Regarding the last point you raise in your comments, on exploring the discrepancies, I agree with you, and, although interesting and welcome, I do not find it strictly necessary to investigate further possible reasons for discrepancy, given that the main results have been replicated.

I think two figures per page is nicer, if they could fit nicely and be readable. I still find it suboptimal to have several pages of figures that completely break off the text, but I don't have an immediate solution for that.

I have one more tiny aesthetic comment: in run_main_simulations.py (and perhaps elsewhere), when lines are broken up then the second part of the line that falls below should be indented.

I will be on holiday and with limited access to internet from next week. My overall recommendation at this point in time is that the replication (with a few smaller edits) should be accepted.

@ChristophMetzner Thank you for your last commits and for your comments. Regarding the last point you raise in your comments, on exploring the discrepancies, I agree with you, and, although interesting and welcome, I do not find it strictly necessary to investigate further possible reasons for discrepancy, given that the main results have been replicated.

I think two figures per page is nicer, if they could fit nicely and be readable. I still find it suboptimal to have several pages of figures that completely break off the text, but I don't have an immediate solution for that.

I have one more tiny aesthetic comment: in run_main_simulations.py (and perhaps elsewhere), when lines are broken up then the second part of the line that falls below should be indented.

I will be on holiday and with limited access to internet from next week. My overall recommendation at this point in time is that the replication (with a few smaller edits) should be accepted.

@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman Jul 25, 2017

@ChristophMetzner Thanks for the commits. I hope to get back to you within the coming week.

@ChristophMetzner Thanks for the commits. I hope to get back to you within the coming week.

@aaronshifman

This comment has been minimized.

Show comment
Hide comment
@aaronshifman

aaronshifman Aug 4, 2017

Hello @ChristophMetzner, I apologize for the delay in the review. I appreciate your addressing my comments and I have no further scientific issues.

My only issues are a few typos / omissions

  • page 1: schizophrenic patientas
  • page 1: loosing much of its simplicity
  • page 11: features a re a little bit
  • page 2, second equation: S_k = \Sigma_j^n ... : n isn't defined in the text (I assume its over all neurons, just a sentence in the text to define it would help)

I agree with @pietromarchesi that the disjointed figures aren't the best option however I don't have a better option and it seems to be that the style that the figure layout isn't nearly as important as the replication itself.

Given this, I move to accept this paper (conditional on these small editorial changes).

Hello @ChristophMetzner, I apologize for the delay in the review. I appreciate your addressing my comments and I have no further scientific issues.

My only issues are a few typos / omissions

  • page 1: schizophrenic patientas
  • page 1: loosing much of its simplicity
  • page 11: features a re a little bit
  • page 2, second equation: S_k = \Sigma_j^n ... : n isn't defined in the text (I assume its over all neurons, just a sentence in the text to define it would help)

I agree with @pietromarchesi that the disjointed figures aren't the best option however I don't have a better option and it seems to be that the style that the figure layout isn't nearly as important as the replication itself.

Given this, I move to accept this paper (conditional on these small editorial changes).

@ChristophMetzner

This comment has been minimized.

Show comment
Hide comment
@ChristophMetzner

ChristophMetzner Aug 10, 2017

@aaronshifman @pietromarchesi I have fixed the typos/omissions and have rearranged the figures and the text and I think that the layout of the article is ok now.

@aaronshifman @pietromarchesi I have fixed the typos/omissions and have rearranged the figures and the text and I think that the layout of the article is ok now.

@rougier rougier added the 02 - Review label Aug 10, 2017

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Aug 30, 2017

Member

@oliviaguest Anything more needed ?

Member

rougier commented Aug 30, 2017

@oliviaguest Anything more needed ?

@oliviaguest

This comment has been minimized.

Show comment
Hide comment
@oliviaguest

oliviaguest Aug 30, 2017

I do not think so. Will move to publish it now.

I do not think so. Will move to publish it now.

@ReScience ReScience locked and limited conversation to collaborators Aug 30, 2017

@oliviaguest

This comment has been minimized.

Show comment
Hide comment

Done! 😄

DOI

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