Skip to content
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 : R. Larisch #57

Closed
wants to merge 65 commits into from
Closed

Conversation

@rLarisch
Copy link

rLarisch commented Apr 10, 2019

AUTHOR

Dear @ReScience/editors,

I request a review for the following replication:

Original article

Title: Connectivity reflects coding: a model of voltage-based STDP with homeostasis
Author(s): Clopath, C., Büsing, L., Vasilaki, E. and Gerstner, W.
Journal (or Conference): Nature Neuroscience
Year: 2010
DOI: 10.1038/nn.2479
PDF: https://www.nature.com/articles/nn.2479

Replication

Author(s): Larisch, R.
Repository: https://github.com/rLarisch/ReScience-submission/tree/Larisch-2019
PDF: https://github.com/rLarisch/ReScience-submission/blob/Larisch-2019/article/Larisch-2019.pdf
Keywords: Spike Timing-Dependent Plasticity (STDP), Computational Neuroscience, Unsupervised Learning
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 (@gdetor)
  • Reviewer 1 (@cJarvers, 27 May 2019)
  • Reviewer 2 (@apdavison, 27 May 2019)
  • Review 1 decision [accept]
  • Review 2 decision [accept]
  • Editor decision [accept]
@cJarvers

This comment has been minimized.

Copy link

cJarvers commented Oct 20, 2019

Sorry for the delay. Here's my review of the updates, based on commit c869eee:

All of my main concerns have been addressed by the updates. Concerning replication, the control experiments (Figure 4 c and d) are now included. It is now much clearer which parts of the implementation were taken from the published Matlab code and how and why certain parameters were changed. Figure 5 and 6 are still not reproduced, but as discussed before I do support publication as a partial replication.

The code is now reproducible thanks to the use of a random seed in the experiment on receptive field formation.

Concerning clarity, I think the paper reads more fluently now. The supplement helps keep an overview over the parameters.

Some minor issues remain, which should be easy to fix:

  • Section 2 of the supplement is empty in the pdf, since the figures are placed on another page. This makes for a weird visual impression. One or two sentences referring to the figures would help.
  • Figure 4a and supplementary figure 2 should better be in greyscale or at least in a linear colour map. The current use of jet makes the figure harder to interpret in general and makes it difficult to read for colourblind people (for a visualization of the problem, see for example this blogpost.
  • The plot of the weights in Figure 4a (Line 138 in Fig4_RF.py) seems to mistakenly use plot instead of imshow, which leads to a strange result for weightsRF.png.
  • Several of the equation references did not get resolved in compilation in the pdf generated with the new template.
  • The references to the Nevian and Sakmann (2006) paper on page 10 still have a @ prefixed to them.
  • The section on reimplementation (page 5) still mentions the two distinct network files net_fix.py and net_homeostatic.py even though the two have been combined.
  • In the pdf generated with the new template, the code blocks have a strange alignment. I guess this is an issue with the template itself.
  • In the section on the voltage clamp experiment (page 8) theta is spelled out and not written as the Greek letter.
@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Oct 24, 2019

Hello @gdetor and @cJarvers,
I have updated the article manuscript to fix the minor issues mentioned by @cJarvers and add the Python protocols with the standard STDP learning rule to the "startAnalysis.py" file.

Still looking forward to the comments of @apdavison.

Best regards,
René Larisch

@gdetor

This comment has been minimized.

Copy link

gdetor commented Oct 24, 2019

@cJarvers Thank you for the review.
@rLarisch Thank you for addressing the comments.
@apdavison Gently reminder.

@apdavison

This comment has been minimized.

Copy link

apdavison commented Oct 29, 2019

I am happy with the changes, in particular running the code locally now produces identical figures to those in the manuscript.

A couple of minor points:

  • as @cJarvers pointed out above, In the section on the voltage clamp experiment (page 8) theta should be \theta
  • the overview mentions Python 3.6, the "Reimplementation" section Python 3.4.

As I mentioned in the original review, I don't think this should be described as a "reference implementation" because of the parameter changes that were needed, but I support publishing it as a partial replication.

@gdetor

This comment has been minimized.

Copy link

gdetor commented Oct 29, 2019

@apdavison Thank you for the review
@rLarisch Could you please address reviewers' comments?

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Oct 30, 2019

@apdavison, @cJarvers

I have fixed the theta in both PDF-files and set the Python version consistent in both files.

@gdetor, it would be fine for me to publish it as a "partial replication", If it is ok, I would change it in the original Request from "fully replicated" to "partially replicated".

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Oct 30, 2019

Hello @gdetor,
I have seen that in the Header of the old ReScience template stands: "A reference implementation of ". Now that the submission is only a partial replication, to what should I change the header? And where?

@rougier

This comment has been minimized.

Copy link
Member

rougier commented Oct 30, 2019

You can explain in the introduction that this is a partial replication of ...

@gdetor

This comment has been minimized.

Copy link

gdetor commented Oct 30, 2019

@apdavison and @cJarvers Are you ok with the final draft?
Thank you

@apdavison

This comment has been minimized.

Copy link

apdavison commented Oct 30, 2019

@gdetor yes

1 similar comment
@cJarvers

This comment has been minimized.

Copy link

cJarvers commented Oct 31, 2019

@gdetor yes

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Nov 1, 2019

@gdetor @rougier I add a sentence at the end of the introduction about the fact that this is only a partial replication.

@gdetor

This comment has been minimized.

Copy link

gdetor commented Nov 4, 2019

@rLarisch Congratulations, your paper has been accepted. I started the publication process, however, I noticed that the license is missing from your home repository. Can you please update the repository?
Thank you @apdavison and @cJarvers for your reviews and valuable comments.

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Nov 5, 2019

@gdetor A LICENCE.txt to the repository. Thank you for your help during the process.

@apdavison and @cJarvers thank you both for your reviews and your constructive comments.

@gdetor gdetor mentioned this pull request Nov 7, 2019
@gdetor

This comment has been minimized.

Copy link

gdetor commented Nov 7, 2019

Hi @rLarisch, because you use the old Rescience template you have to update the metadata on your side. I attach the corresponding metadata.yaml file that you have to use (I have filled out all the necessary information).
metadata.txt
Don't forget to rename the metadata.txt to metadata.yaml
Thank you.

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Nov 8, 2019

Hi @gdetor, there was a metadata.yaml file in the article/new_template/ directory. I have now paste your metadata.yaml file in the article/ directory and have updated the .yaml file in the article/new_template/ directory. I hope this is ok.

@gdetor

This comment has been minimized.

Copy link

gdetor commented Nov 8, 2019

Hi, @rLarisch thank you for updating the repo, however, you have to regenerate the pdf (new template) using the metadata file I sent you since it has all the necessary information for publishing the article. Thank you.

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Nov 11, 2019

Hi @gdetor, I have updated the article.pdf from the new template. Should I update the corresponding informations in the *.md file from the old template too ?

@gdetor

This comment has been minimized.

Copy link

gdetor commented Nov 11, 2019

@rLarisch It would be nice to have it for consistency. Furthermore, I would like to ask you to regenerate the pdf with the following metadata file (I confused the sandbox metadata with the Zenodo one). The attached one is the final. Thank you and my apologies for the inconvenience.
metadata.txt

@rLarisch

This comment has been minimized.

Copy link
Author

rLarisch commented Nov 12, 2019

Hi @gdetor, the article.pdf with the new metadata.yaml file is updated and the corresponding information in the old *.md entered and the corresponding pdf fresh compiled.

@gdetor

This comment has been minimized.

Copy link

gdetor commented Nov 14, 2019

Hi @rLarisch, your article is now online http://rescience.github.io/bibliography/Larisch_2019.html
This issue is now closing.

@gdetor gdetor closed this Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.