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: Detorakis #35

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@gdetor

gdetor commented Aug 2, 2017

AUTHOR

Dear @ReScience/editors,

I request a review for the following replication:

Original article

Title: A Generalized Linear Integrate-and-Fire Neural Model Produces Diverse Spiking Behaviors
Author(s): Stefan Mihalas and Ernst Niebur
Journal (or Conference): Neural Computation
Year: 2009
DOI: 10.1162/neco.2008.12-07-680
PDF: https://www.ncbi.nlm.nih.gov/pmc/articles/PMC2954058/
(or https://capocaccia.ethz.ch/capo/raw-attachment/wiki/2010/spinn10/Mihalas%20Niebur%20NECO.pdf)

Replication

Author(s): Georgios Detorakis
Repository: https://github.com/gdetor/ReScience-submission/tree/detorakis-2017
PDF: https://github.com/gdetor/ReScience-submission/blob/detorakis-2017/article/detorakis-2017.pdf
Keywords: linear leaky integrate-and-fire neuron, diverse spiking behavior, differential equations
Language: Python
Domain: Computational Neuroscience

Results

  • [ x] Article has been fully replicated
  • Article has been partially replicated
  • Article has not been replicated

Potential reviewers


EDITOR

  • Editor acknowledgment (@otizonaizit) 3 August 2017
  • Reviewer 1 (@heplesser) 3 August 2017
  • Reviewer 2 (@pdebuyl) 3 August 2017
  • Review 1 decision [accept] 18 August 2017
  • Review 2 decision [accept] 14 August 2017
  • Editor decision [accept] 18 August 2017
@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Aug 2, 2017

Member

@gdetor Thanks for your submission. Do you have a link on the original PDF?

@otizonaizit Can you handle the submission? (I know you're busy, no problem if you can't)

Member

rougier commented Aug 2, 2017

@gdetor Thanks for your submission. Do you have a link on the original PDF?

@otizonaizit Can you handle the submission? (I know you're busy, no problem if you can't)

@rougier rougier changed the title from New submission to Review request: Detorakis Aug 2, 2017

@gdetor

This comment has been minimized.

Show comment
Hide comment
@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Aug 3, 2017

Member

@gdetor Would you have a free preprint instead ?

Member

rougier commented Aug 3, 2017

@gdetor Would you have a free preprint instead ?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 3, 2017

@rougier I would volunteer to review.

heplesser commented Aug 3, 2017

@rougier I would volunteer to review.

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 3, 2017

Member
Member

otizonaizit commented Aug 3, 2017

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 3, 2017

Member

@heplesser : thank you! I assigned you as reviewer.

Member

otizonaizit commented Aug 3, 2017

@heplesser : thank you! I assigned you as reviewer.

@otizonaizit otizonaizit requested a review from pdebuyl Aug 3, 2017

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 3, 2017

Member

@pdebuyl : can you review this?

Member

otizonaizit commented Aug 3, 2017

@pdebuyl : can you review this?

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Aug 3, 2017

Member

I can do this next week.

Member

pdebuyl commented Aug 3, 2017

I can do this next week.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 4, 2017

@gdetor @otizonaizit I find you replication successful and your manuscript well written. I would like to request a few minor improvements to text and code before approving:

  1. You mention only in the Conclusion how you inferred those parameters not explicitly given by Mihalas and Niebur. I think it would be better to provide this information already under Methods.
  2. Your tables are organized slightly differently than the table in Mihalas and Niebur. It would aid the reader if you mentioned the differences briefly and stated explicitly that parameter values are identical to those in the original paper.
  3. In your table 4, a parameter t_f appears that is not explained in the text. It appears to be the length of the trace shown in Fig 1 for each case, but needs to be explained.
  4. In Fig 1, you should place the case labels A, B, C, ... outside the frame in black and ensure that there is a tick label at the right edge of the x-axis for each frame. To avoid confusing the reader, you should also point out that you use the same y-axis limits throughout, while the original figure used different limits for each case.
  5. Could you add flow vectors to Fig 2?
  6. I think your Fig 3 is more informative than the original one, but you should maybe point out that your figure is oriented differently from Fig 3 in the original paper.
  7. You mnn_model() code mixes model simulation and plotting concerns. I would strongly suggest that mnn_model() should only simulate the model and that "prettification" is handled elsewhere.
  8. External current is supplied via the pms dictionary as an array of current values; this array implicitly determines the duration of the simulation and assumes a certain time step. Providing simulation duration and time step as separate variables risks inconsistencies. The tidiest solution is probably to provide I_ext and dt as arguments to mnn_model() and infer time. I also do not quite understand why you build T incrementally as a list instead of creating it as a NumPy array right away.
  9. run_all would be a bit more legible if you defined a function that creates each single plot and then called that function for each case, instead of repeating the same code over and over.

heplesser commented Aug 4, 2017

@gdetor @otizonaizit I find you replication successful and your manuscript well written. I would like to request a few minor improvements to text and code before approving:

  1. You mention only in the Conclusion how you inferred those parameters not explicitly given by Mihalas and Niebur. I think it would be better to provide this information already under Methods.
  2. Your tables are organized slightly differently than the table in Mihalas and Niebur. It would aid the reader if you mentioned the differences briefly and stated explicitly that parameter values are identical to those in the original paper.
  3. In your table 4, a parameter t_f appears that is not explained in the text. It appears to be the length of the trace shown in Fig 1 for each case, but needs to be explained.
  4. In Fig 1, you should place the case labels A, B, C, ... outside the frame in black and ensure that there is a tick label at the right edge of the x-axis for each frame. To avoid confusing the reader, you should also point out that you use the same y-axis limits throughout, while the original figure used different limits for each case.
  5. Could you add flow vectors to Fig 2?
  6. I think your Fig 3 is more informative than the original one, but you should maybe point out that your figure is oriented differently from Fig 3 in the original paper.
  7. You mnn_model() code mixes model simulation and plotting concerns. I would strongly suggest that mnn_model() should only simulate the model and that "prettification" is handled elsewhere.
  8. External current is supplied via the pms dictionary as an array of current values; this array implicitly determines the duration of the simulation and assumes a certain time step. Providing simulation duration and time step as separate variables risks inconsistencies. The tidiest solution is probably to provide I_ext and dt as arguments to mnn_model() and infer time. I also do not quite understand why you build T incrementally as a list instead of creating it as a NumPy array right away.
  9. run_all would be a bit more legible if you defined a function that creates each single plot and then called that function for each case, instead of repeating the same code over and over.
@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 4, 2017

Member

@pdebuyl : thank you for accepting to review! Next week is perfectly fine, take your time…

Member

otizonaizit commented Aug 4, 2017

@pdebuyl : thank you for accepting to review! Next week is perfectly fine, take your time…

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 4, 2017

Member

@heplesser : thank you for the thorough review!
@gdetor : you can start addressing @heplesser comments already :-)

Member

otizonaizit commented Aug 4, 2017

@heplesser : thank you for the thorough review!
@gdetor : you can start addressing @heplesser comments already :-)

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Aug 10, 2017

Member

Dear @gdetor, @otizonaizit,

The reproduction by @gdetor is well executed and runs as expected on my computer. The code
dependencies are all standard, suggesting easy re-use.

I have a list of comments that I believe should be addressed before accepting the article:

  1. In the original manuscript, as far I can read, the solutions are obtained in Eq. 3.2 with
    the next firing time obtained in Eq. 3.5. I used the pdf at
    https://www.ncbi.nlm.nih.gov/pmc/articles/PMC2954058/pdf/nihms211651.pdf , in the file
    https://capocaccia.ethz.ch/capo/raw-attachment/wiki/2010/spinn10/Mihalas%20Niebur%20NECO.pdf
    these are eqs. 4 and 7. I understand very well the rationale to choose a fixed time step
    ODE solution here but this choice and the different technique with respect to the
    original manuscript should be commented on explicitly.

  2. The license for the whole repo is GPLv3. It is not suitable for the article that should
    be licensed under CC-BY.

  3. The code/README.md file does not contain any information. It should contain a brief
    description of the Python files and the platform
    information
    that is requested since several months
    now by ReScience.

  4. The first argument to the model integration routine is a mutable. In the case where one
    runs the code with ipython -i or via the %run IPython magic, the defaults will be
    those given by the conditional execution after the line if __name__ == '__main__'. This
    is a likely source of confusion and bugs for users of the code.

  5. The main result of the paper is figure 1. Although the layout in a 4x5 matrix is intended
    to follow the original paper, it makes the figure's content unsuitably small. I suggest
    to change this by using a 2x10 matrix or splitting the figure, for instance.

  6. A check could be added in mnn_model to ensure that the length of Iext is sufficient.

Member

pdebuyl commented Aug 10, 2017

Dear @gdetor, @otizonaizit,

The reproduction by @gdetor is well executed and runs as expected on my computer. The code
dependencies are all standard, suggesting easy re-use.

I have a list of comments that I believe should be addressed before accepting the article:

  1. In the original manuscript, as far I can read, the solutions are obtained in Eq. 3.2 with
    the next firing time obtained in Eq. 3.5. I used the pdf at
    https://www.ncbi.nlm.nih.gov/pmc/articles/PMC2954058/pdf/nihms211651.pdf , in the file
    https://capocaccia.ethz.ch/capo/raw-attachment/wiki/2010/spinn10/Mihalas%20Niebur%20NECO.pdf
    these are eqs. 4 and 7. I understand very well the rationale to choose a fixed time step
    ODE solution here but this choice and the different technique with respect to the
    original manuscript should be commented on explicitly.

  2. The license for the whole repo is GPLv3. It is not suitable for the article that should
    be licensed under CC-BY.

  3. The code/README.md file does not contain any information. It should contain a brief
    description of the Python files and the platform
    information
    that is requested since several months
    now by ReScience.

  4. The first argument to the model integration routine is a mutable. In the case where one
    runs the code with ipython -i or via the %run IPython magic, the defaults will be
    those given by the conditional execution after the line if __name__ == '__main__'. This
    is a likely source of confusion and bugs for users of the code.

  5. The main result of the paper is figure 1. Although the layout in a 4x5 matrix is intended
    to follow the original paper, it makes the figure's content unsuitably small. I suggest
    to change this by using a 2x10 matrix or splitting the figure, for instance.

  6. A check could be added in mnn_model to ensure that the length of Iext is sufficient.

@gdetor

This comment has been minimized.

Show comment
Hide comment
@gdetor

gdetor Aug 10, 2017

Dear @otizonaizit @heplesser @pdebuyl ,
Thank you for your corrections/comments and suggestions. All the issues have been addressed. The reviewed manuscript and the source codes have been updated accordingly.

Thank you,
Georgios

gdetor commented Aug 10, 2017

Dear @otizonaizit @heplesser @pdebuyl ,
Thank you for your corrections/comments and suggestions. All the issues have been addressed. The reviewed manuscript and the source codes have been updated accordingly.

Thank you,
Georgios

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Aug 14, 2017

Member

Dear @gdetor,

Thank you for the update. You modified the article license to BSD, it really should be CC-BY :-)

git show master:code/LICENSE.txt > code/LICENSE.txt

will give you the original one that is totally fine. BSD is of course fine for the code
directory.

The problem of the params mutable is still of relevance. It is now avoided in the specific
cases I mentioned in my earlier report but can still bite anyone re-using params after
importing the module neuron_model. A method such as "default_params" that creates and
returns a copy of a the parameters dictionary would be much safer, along with a
params=None default-valued argument that would call this method when it is indeed
None. The default-parameters-getter could also override the values with keywords arguments
to save typing later on:

pms = neuron_model.default_parameters(a=0.005)

All my other concerns were addressed suitably.

Member

pdebuyl commented Aug 14, 2017

Dear @gdetor,

Thank you for the update. You modified the article license to BSD, it really should be CC-BY :-)

git show master:code/LICENSE.txt > code/LICENSE.txt

will give you the original one that is totally fine. BSD is of course fine for the code
directory.

The problem of the params mutable is still of relevance. It is now avoided in the specific
cases I mentioned in my earlier report but can still bite anyone re-using params after
importing the module neuron_model. A method such as "default_params" that creates and
returns a copy of a the parameters dictionary would be much safer, along with a
params=None default-valued argument that would call this method when it is indeed
None. The default-parameters-getter could also override the values with keywords arguments
to save typing later on:

pms = neuron_model.default_parameters(a=0.005)

All my other concerns were addressed suitably.

@gdetor

This comment has been minimized.

Show comment
Hide comment
@gdetor

gdetor Aug 14, 2017

Dear @pdebuyl
Thank you for the comments. I changed the license to CC-BY (the license for the code though is now GPL2). Furthermore, I add a function that generates a default parameters dictionary and takes keyword arguments as you suggested.

gdetor commented Aug 14, 2017

Dear @pdebuyl
Thank you for the comments. I changed the license to CC-BY (the license for the code though is now GPL2). Furthermore, I add a function that generates a default parameters dictionary and takes keyword arguments as you suggested.

@heplesser

@gdetor Thank you for your changes. I just have a few minor language- and code-related issues left, see my inline comments.

Show outdated Hide outdated article/detorakis-2017.tex
Show outdated Hide outdated article/detorakis-2017.tex
Show outdated Hide outdated article/detorakis-2017.tex
Show outdated Hide outdated article/detorakis-2017.tex
Show outdated Hide outdated code/neuron_model.py
Show outdated Hide outdated code/neuron_model.py
@gdetor

This comment has been minimized.

Show comment
Hide comment
@gdetor

gdetor Aug 16, 2017

@heplesser Thank you for your corrections and comments. All the issues have beed addressed.

gdetor commented Aug 16, 2017

@heplesser Thank you for your corrections and comments. All the issues have beed addressed.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Aug 17, 2017

@gdetor Thank you for your updates! After your last commit f6f3165, lines 70ff of neuron_model.py read

def mnn_model(pms=None, Iext=np.zeros((200,)), dt=1.0,
              IC=(0.01, 0.001, -70.0, -50.0)):

so the mutable np.zeros((200,)) is still there as default argument.
https://github.com/ReScience/ReScience-submission/pull/35/files#diff-8a081cf2e57010471dacd32d1e3805e6R70

heplesser commented Aug 17, 2017

@gdetor Thank you for your updates! After your last commit f6f3165, lines 70ff of neuron_model.py read

def mnn_model(pms=None, Iext=np.zeros((200,)), dt=1.0,
              IC=(0.01, 0.001, -70.0, -50.0)):

so the mutable np.zeros((200,)) is still there as default argument.
https://github.com/ReScience/ReScience-submission/pull/35/files#diff-8a081cf2e57010471dacd32d1e3805e6R70

@gdetor

This comment has been minimized.

Show comment
Hide comment
@gdetor

gdetor Aug 17, 2017

@heplesser Thank you for noticing that. I updated the code accordingly.

gdetor commented Aug 17, 2017

@heplesser Thank you for noticing that. I updated the code accordingly.

@heplesser

@gdetor Thank you for your corrections!
@otizonaizit I approve the manuscript and code for publication in ReScience.

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 18, 2017

Member

@gdetor Congratulations, I hereby accept the submission for publication!
@heplesser , @pdebuyl thank you for your reviews!

Member

otizonaizit commented Aug 18, 2017

@gdetor Congratulations, I hereby accept the submission for publication!
@heplesser , @pdebuyl thank you for your reviews!

@pdebuyl

This comment has been minimized.

Show comment
Hide comment
@pdebuyl

pdebuyl Aug 18, 2017

Member

Before publishing, I'd just like to request that the edits are made in the markdown file and not the latex file. If ReScience decides at some point to publish html versions of the articles this will be mandatory.

Member

pdebuyl commented Aug 18, 2017

Before publishing, I'd just like to request that the edits are made in the markdown file and not the latex file. If ReScience decides at some point to publish html versions of the articles this will be mandatory.

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 18, 2017

Member

@pdebuyl good catch! Thanks for pointing this out. @gdetor can you take care of this before we proceed?

Member

otizonaizit commented Aug 18, 2017

@pdebuyl good catch! Thanks for pointing this out. @gdetor can you take care of this before we proceed?

@gdetor

This comment has been minimized.

Show comment
Hide comment
@gdetor

gdetor Aug 22, 2017

@otizonaizit I fixed the markdown file accordingly. Let me know if you need anything more.
Thank you.

gdetor commented Aug 22, 2017

@otizonaizit I fixed the markdown file accordingly. Let me know if you need anything more.
Thank you.

@rougier

This comment has been minimized.

Show comment
Hide comment
@rougier

rougier Aug 30, 2017

Member

@otizonaizit Do you need some help for the publication (I know you're busy right now)?

Member

rougier commented Aug 30, 2017

@otizonaizit Do you need some help for the publication (I know you're busy right now)?

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Aug 30, 2017

Member
Member

otizonaizit commented Aug 30, 2017

@ReScience ReScience locked and limited conversation to collaborators Sep 6, 2017

@ReScience ReScience unlocked this conversation Sep 6, 2017

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Sep 6, 2017

Member

@gdetor : we have problems compiling your file, the tex file is corrupted, some headers are wrong, the bibliography is included in the md file, etc... can you try fixing the issues and testing that you can generate a tex file with pandoc from the md file and then compile the tex to pdf?

Member

otizonaizit commented Sep 6, 2017

@gdetor : we have problems compiling your file, the tex file is corrupted, some headers are wrong, the bibliography is included in the md file, etc... can you try fixing the issues and testing that you can generate a tex file with pandoc from the md file and then compile the tex to pdf?

@otizonaizit

This comment has been minimized.

Show comment
Hide comment
@otizonaizit

otizonaizit Oct 6, 2017

Member

Article is published with DOI

The article will be soon listed on http://rescience.github.io/read/

Member

otizonaizit commented Oct 6, 2017

Article is published with DOI

The article will be soon listed on http://rescience.github.io/read/

@otizonaizit otizonaizit closed this Oct 6, 2017

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