Skip to content

Review Request: Detorakis #17

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

Closed
wants to merge 11 commits into from
Closed

Conversation

gdetor
Copy link

@gdetor gdetor commented May 6, 2016

Dear @ReScience/editors,

I request a review for the reproduction of the following paper:

  • Multiple dynamical modes of thalamic relay neurons: rhythmic bursting and
    intermittent phase-locking, Wang, X-J, Neuroscience, 59(1), pg. 21–31, 1994.

I believe the original results have been faithfully reproduced as explained in the accompanying article.

The repository lives @ https://github.com/gdetor/ReScience-submission/tree/detorakis

Best regards,
Georgios Detorakis


EDITOR

  • Editor acknowledgment (@otizonaizit) May 9, 2016
  • Reviewer 1 (@heplesser) May 10, 2016
  • Reviewer 2 (@apdavison) May 9, 2016
  • Review 1 decision [accept] Aug 9, 2016
  • Review 2 decision [accept] Aug 28, 2016
  • Editor decision [accept] Aug 29, 2016

@rougier
Copy link
Member

rougier commented May 6, 2016

@gdetor Thanks, an editor will be soon assigned.
@otizonaizit Can you handle this request (I've a conflict of interest) ?

@otizonaizit
Copy link
Member

otizonaizit commented May 9, 2016

EDITOR
@rougier: yep, I am on it.

@otizonaizit
Copy link
Member

EDITOR
@heplesser: Can you be REVIEWER 1 for this?
@apdavison: Can you be REVIEWER 2 for this?

@apdavison
Copy link

apdavison commented May 9, 2016

REVIEWER 2
@otizonaizit yes, I'd be happy to review this.

@FedericoV
Copy link

I might also be able to help if needed. It's Python and differential
equations.

On Mon, 9 May 2016 at 11:54 Tiziano Zito notifications@github.com wrote:

EDITOR
@heplesser https://github.com/heplesser: Can you be REVIEWER 1 for
this?
@apdavison https://github.com/apdavison: Can you be REVIEWER 2 for
this?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#17 (comment)

@otizonaizit
Copy link
Member

otizonaizit commented May 9, 2016

Thanks Federico, I'd let @heplesser a bit of time to accept the review.
In case he is not available for this review I'll come back to your
offer.

On Mon 09 May, 04:11, Federico Vaggi notifications@github.com wrote:

I might also be able to help if needed. It's Python and differential
equations.

On Mon, 9 May 2016 at 11:54 Tiziano Zito notifications@github.com wrote:

EDITOR
@heplesser https://github.com/heplesser: Can you be REVIEWER 1 for
this?
@apdavison https://github.com/apdavison: Can you be REVIEWER 2 for
this?


You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#17 (comment)


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#17 (comment)

@heplesser
Copy link

heplesser commented May 9, 2016

REVIEWER 1
@otizonaizit I'd be happy to review this.

@rougier rougier changed the title Review Request Review Request: Detorakis May 10, 2016
@otizonaizit
Copy link
Member

EDITOR
@gdetor: your paper is under review by @heplesser (REVIEWER 1) and @apdavison (REVIEWER 2). Stay tuned ;)

@heplesser
Copy link

heplesser commented May 12, 2016

REVIEWER 1

@gdetor I will begin with a few remarks and return with more detailed comments later.

First of all, your code ran out of the box and created the figures included in your manuscript.

While I think that you are quite close to re-implementing Wang's model (I have not yet compared the equations from the paper in detail to your code), I believe that more effort is needed to establish the quality of your model thoroughly. My understanding of the idea behind ReScience is that your implementation of Wang's model would become the gold standard for anyone who would want to use this model, including reimplementation in other software. Therefore, it is, in my opinion, essential that you verify as exactly as possible that your implementation reproduces Wang's results. Where discrepancies occur, they need to be analyzed and explained in detail.

Specifically, I think that it would much strengthen your model if you would recreate all figures and Table 1. To judge the quality of a model re-implementation, it is particularly interesting to see whether it responds to parameter changes in the same way as the original. Fig 1, bottom, Fig 2C, Table 1, and Fig 7, seem particularly relevant in that respect.

You write that you needed to adjust some parameter values slightly to match Wang's figures, and speculate that this may be due to your use of a different integration method. This is problematic. Clearly, it might be that some of the details of the responses shown in Wang's paper are as they are due to the specific numeric method used, and it might very well be that the method you use is better, in the sense that the solutions you obtain are closer to the mathematically correct solution. But it might also be due to other effects.

Therefore, I think it is important to first try to reproduce Wang as closely as possible, using the same integrator used originally. The precise step-size control is not given in the paper, but you could easily try the dopri5 4(5) order RK that comes with scipy.integrate.ode. This would allow you to explore to which degree your results depend on the integration method used. In general, I'd suggest that you allow the user to specify the integration method for easier testing. Where parameter adjustments are required, you should make explicit which parameters were changed from which value to which and with which effect.

@rougier rougier assigned otizonaizit and unassigned rougier May 14, 2016
@otizonaizit
Copy link
Member

EDITOR
@apdavison: any chance you can review this by the end of the week? If not, let me know when you'll have time :)

@apdavison
Copy link

apdavison commented May 18, 2016

REVIEWER 2
I've started the review (the code runs fine and creates the figures in the manuscript) but I'm at a conference tomorrow and Friday so can't finish it this week. I should have time on Monday (23rd).

@otizonaizit
Copy link
Member

EDITOR
@apdavison: great, thank you!



def loadParameters(fname):
""" Load all the necessary paremeters from a file.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@apdavison
Copy link

apdavison commented May 23, 2016

REVIEWER 2
The code ran without first time without problems, and a visual inspection shows a clean, easy-to-read structure which is ideal for a reference implementation.

I am not, however, convinced that the results are quantitatively comparable to the original.

  1. The time axis in Figure 2 is 35 seconds in size. In the original Figure 3, however, the scale bar is 100 ms, which suggests the entire x-axis is about 1 second.
  2. It is difficult to compare the scales of Figure 3 and the original Figure 6, since the size of the axes is different. Figure 3 should be replotted on the same axes as the original.
  3. It is not sufficient to say that the integration method was different, and some of the parameters had to be changed to get the results to agree. If possible, the same integration method as the original, or one which is similar to it, should be used. If this would require extraordinary efforts, then it should at least be demonstrated that the results are consistent when (i) using two different integration methods and (ii) reducing the integration time step by a factor of ten.
  4. I do not think that reproduction of three figures showing membrane potential traces is sufficient to claim reproduction. Either Table 1 or Figure 7 or both should also be reproduced.

Three minor comments on the typesetting of the paper:

  1. there should always be a space between the numerical value and unit symbol (see http://physics.nist.gov/cuu/Units/checklist.html point 15) (I find the LaTeX "small space", \,, works well);
  2. unit symbols should be in roman type (http://physics.nist.gov/cuu/Units/checklist.html point 6) (e.g. \mathrm{mV})
  3. Subscripts should be in roman type if they are descriptive (e.g. I_{\mathrm{K}} since K is for potassium and is not a variable name). Italic type is for subscripts which represent variables (e.g. \beta_{n} is fine, since n is a variable of the model). (http://physics.nist.gov/cuu/Units/checklist.html point 7).

@otizonaizit
Copy link
Member

EDITOR
@heplesser, @apdavison: thank you for your reviews!
@gdetor: Can you address the reviewers' comments?

@gdetor
Copy link
Author

gdetor commented May 23, 2016

@apdavison @heplesser Thank you for the comments/suggestions.
@otizonaizit I already started addressing reviewers comments. Once everything's ready I will commit the changes. Thank you.

@rougier
Copy link
Member

rougier commented Jun 7, 2016

@gdetor @otizonaizit Any progress ?

@gdetor
Copy link
Author

gdetor commented Jun 7, 2016

I'm still running some long-running simulations in order to validate the model. I estimate by the end of this week, I will submit the updated version.

@otizonaizit
Copy link
Member

EDITOR

@heplesser, @rougier: I think that the manuscript can be considered to successfully replicate the original paper, provided the authors address @heplesser comments either implementing his suggestions or explaining why they can't be implemented. I am still waiting for @apdavison final recommendation to be able to come to a decision, but given the tone of his review I think we are very close to a positive outcome, @gdetor.

@apdavison
Copy link

I recommend acceptance. Concerning Figure 2, I doubt it is possible to more closely reproduce the original Fig 1 in the absence of information about which method was used to produce the smooth curves, although I agree with @heplesser that some comment should be made about this in the manuscript.

@gdetor
Copy link
Author

gdetor commented Aug 18, 2016

Dear all,
Thank you again for your comments/suggestions/corrections. All the comments have been addressed (code, text, and figures). I think that now almost all of the figures are quite close to the original ones except my second one (first figure in Wang). I tried a high resolution (100 x 100) discretization of the parameter space, and now the curves are smoother than previous ones but still there are some differences.

Best regards,
Georgios Detorakis

@otizonaizit
Copy link
Member

otizonaizit commented Aug 19, 2016

EDITOR
@gdetor: congratulations, I hereby accept the submission
@heplesser, @apdavison: thanks to the reviewers for their helpful comments, I think they helped in improving the paper significantly.

@gdetor, @rougier: I am currently mostly offline or with very flaky internet connection. I'll publish the paper in two weeks when I'm back to civilization if this is fine with you.

@rougier
Copy link
Member

rougier commented Aug 20, 2016

@otizonaizit Fine for me. The publication process is still not straightforward so you might need my help but it would be good to have your comments on what could be modified.

@otizonaizit
Copy link
Member

Hi @gdetor, Reviewer 1 noticed that I did not wait for his recommendation before deciding, and I think his further comments to the manuscript are definitely worth addressing. @heplesser: can you please post your further comments? @gdetor: could you please address @heplesser comments so that we can finally accept and publish the paper? Thanks!

@gdetor
Copy link
Author

gdetor commented Aug 22, 2016

@otizonaizit Sure, I'll wait for the comments.

@heplesser
Copy link

@gdetor I believe that the paper is essentially sound and should be published, but there are a few things to fix:

In the new Figure 1, the horizontal axis does not show "Error", but "Voltage Difference" between the solutions. The unit of measurement should be given, which, I believe, is mV here. On the vertical axis, the unit should not be Hz, which applies to periodic processes only. If I read the code correctly, data is taken by binning values from simulation over 6000 ms into 30 bins, so what is shown is number of occurances of voltage differences over 6 seconds. I am also not sure that this diagram really provides useful information. Maybe it would be better to plot one solution as function of the other solution. In case of perfect agreement, one would just get a straight line along the diagonal, deviations show where/how the different solvers behave differently.

I am also not sure what you mean by "overlapping spike events".

Figure 6 now looks much better than in the previous version, in fine agreement with the original. From the changes in code/params/params_figure6a.cfg

-phi_K = 28.6
+phi_K = 28.5714285714

it appears that this very delicate adjustment in phi_K was required to reach agreement (phi_K is phi_n in the paper). The latter value is indeed the correct decimal value for 200/7 from the paper (it would be useful to add this as a comment in the config file). But I think it would be very useful for readers and people who work with the code to know that a change in phi_K by about 0.1% will have distinct effects on the behavior of the neuron.

Furthermore, Table 5 of the manuscript gives a value of $\phi_n = 28.5$. That value is according to the code/params/*.cfg files used for figures 1, 3 and 5, while figure 2 uses 28.6, and figure 4, 6 and 7 use 200/7. Given the significant effect of minuscule changes, this is not acceptable. All figures should be based on 200/7, and that value should be given in the Table (Btw, does your config-file format allow for comments? It is not immediately obvious that 200/7 == 28.5714285714)

This raises another concern: Are all other parameters correct as given in the Tables?

Finally, a figure reference is incorrect (Figure ??) in the conclusions, and the English in the parts of the text that came in with the most recent revision would benefit from some polishing.

@gdetor
Copy link
Author

gdetor commented Aug 25, 2016

Dear all,
I've just committed new corrections according to reviewers comments.

Furthermore,

  1. In the new Figure 1, the horizontal axis does not show "Error", but "Voltage Difference" between the solutions. The unit of measurement should be given, which, I believe, is mV here. On the vertical axis, the unit should not be Hz, which applies to periodic processes only. If I read the code correctly, data is taken by binning values from simulation over 6000 ms into 30 bins, so what is shown is number of occurances of voltage differences over 6 seconds. I am also not sure that this diagram really provides useful information. Maybe it would be better to plot one solution as function of the other solution. In case of perfect agreement, one would just get a straight line along the diagonal, deviations show where/how the different solvers behave differently.

Figure 1 has now changed according to reviewer's suggestion.

  1. Figure 6 now looks much better than in the previous version, in fine agreement with the original. From the changes in code/params/params_figure6a.cfg

-phi_K = 28.6
+phi_K = 28.5714285714
it appears that this very delicate adjustment in phi_K was required to reach agreement (phi_K is phi_n in the paper). The latter value is indeed the correct decimal value for 200/7 from the paper (it would be useful to add this as a comment in the config file). But I think it would be very useful for readers and people who work with the code to know that a change in phi_K by about 0.1% will have distinct effects on the behavior of the neuron.

The problem was not the value of phi (since I tried several simulations wth both values 28.6 and 28.5714285714) but the total simulation time. The signal was too short for any further analysis. By increasing simulation time everything was better.

  1. Furthermore, Table 5 of the manuscript gives a value of $\phi_n = 28.5$. That value is according to the code/params/*.cfg files used for figures 1, 3 and 5, while figure 2 uses 28.6, and figure 4, 6 and 7 use 200/7. Given the significant effect of minuscule changes, this is not acceptable. All figures should be based on 200/7, and that value should be given in the Table (Btw, does your config-file format allow for comments? It is not immediately obvious that 200/7 == 28.5714285714)

Thank you for these comments. I found out that due to previous corrections and modifications some of the values in Table 5 were wrong. I double checked and corrected all the wrong values.

@heplesser
Copy link

@gdetor Thank you very much for your revisions! I must admit it is a relieve to see that the difference in Fig 6 was due to simulation time, not due to tiny changes in phi_K. I think it would be useful to point out in the discussion of that figure (and of figure 2) that one needs to collect enough data to get results close to the originals. This is a valuable insight from your reproduction efforts and should not be hidden just in a table.

Concerning Table 2, I would us "Simulated times", not "Simulation times". The latter could be misunderstood as the time it took to run the simulation. For Fig 2, it should be $15\times\text{period}$, to that "period" is set as roman text.

Concerning Fig 1: It makes things look much worse than they are because you are connecting the dots. If you don't, it looks much better (see attached notebook); there, I shade an area +-2mV from the diagonal in addition.

But note that you code for checking correctness of spikes is not correct:

(dopri[spks1,0]-adams[spks2,0]).sum())

The sum always ends up zero, even though there are differences in spike times. For Adams vs DoPri5, only a single spike is one time step (0.05 ms) late, while for BDF vs DoPri5 more than half the spikes are 0.05ms late. But the conclusion seems sensible that Adams gives comparable spike trains and can be used.

detorakis_test.ipynb.zip

@gdetor
Copy link
Author

gdetor commented Aug 26, 2016

Thank you again for those comments. I already addressed them and I committed the new files.

@heplesser
Copy link

@otizonaizit @gdetor Thank you for the revision. The paper now successfully reproduces the original results and I recommend acceptance. I just suggest two small language fixes. On p 2., "The most stroking difference found for the amplitude of membrane potential" should be "The most striking difference is found ...", and in Table 2, also the table caption should be changed to "Simulated time".

@gdetor
Copy link
Author

gdetor commented Aug 29, 2016

Dear all,
Thank you once again. I corrected the two typos and the final version is now in the repository.

@otizonaizit
Copy link
Member

EDITOR
@gdetor: congratulations, I hereby accept the submission, this time for real ;-)
@heplesser, @apdavison: thanks to the reviewers again for their helpful comments.

@gdetor, @rougier: I am currently mostly offline or with very flaky internet connection. I'll publish the paper in next week when I'm back to civilization if this is fine with you.

@ReScience ReScience locked and limited conversation to collaborators Sep 7, 2016
@otizonaizit
Copy link
Member

@gdetor: can you give us some keywords to use for the publication? Right now I have: Neuroscience, Python, Replication. Some more detailed keyword would help. Thanks!

@otizonaizit
Copy link
Member

This submission has been accepted for publication, and has been published and appeared at
http://rescience.github.io/read/

DOI

@otizonaizit otizonaizit closed this Sep 7, 2016
@gdetor
Copy link
Author

gdetor commented Sep 7, 2016

@otizonaizit Some extra keywords: Conductance-based model, Thalamic relay neurons, Intermittent phase-locking, Spindle oscillation, Delta oscillation

@damiendr
Copy link

damiendr commented Sep 7, 2016

Just noticed that the published PDF does not include the last commit for the typos.

@rougier
Copy link
Member

rougier commented Sep 7, 2016

@damiendr Thanks !
@otizonaizit Do you where we messed up things ?

@rougier
Copy link
Member

rougier commented Sep 8, 2016

@damiendr Should be fixed by now.

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

Successfully merging this pull request may close these issues.

7 participants