Closed sepehrmn closed 4 years ago
I think that is everything required to start the process.
This authors of this paper used classical information theory. The nascent field of Partial Information Decomposition (PID) enables breaking three-way (and some formulations with more terms) information theory into more precise parts. Since this is a journal for replicating results of old studies, I have limited the scope to that and not discussed any of this in the submission or included any PIDs here, but that can change if desired.
@sepehrmn Sorry for the delay, the Ten Years Reproducibility Challenge has made us quite busy these past days.
@sepehrmn For figure 2, yu might want to put sub-figures side by side and try to fix the aspect ratio. Currently, they're a bit difficult to read.
@gdetor @oliviaguest @benoit-girard @eroesch Can any of you take care of this regular submission in computational neuroscience ?
Happy to take this on. 😄
@oliviaguest If you don't mind, I'd be happy to act as a reviewer for this submission. Would be my first one then. 🙂
@oliviaguest Thank you @schmidDan Thank you, I guess you can start your review but @oliviaguest will confirm it.
@schmidDan given you have only a couple repos and they are both forks, can you link me to your institutional profile and/or website, please? It will help me decide on who else to invite and understand (of course) your skills. Thanks.
@oliviaguest thanks for the fast reply!
Since I'm starting out with my PhD studies at the moment, my institutional website is still to become available. My affiliation, though, is with the Institute for Neural Information Processing, Ulm University, Germany. The closest thing I can link for reference would be my ORCID. Regarding public repos you're right. So far repos showcasing my work aren't available to the public. Something I hopefully can change throughout the year. My skills listed on ReScience's list of reviewers is up-to-date so far: I'm familiar with Python, Julia and C++. Besides, this replication study is of specific interest to me as I familiarized myself in the past with the work of Phillips, Kay, Wibral and their co-workers on contextual modulation in neural processors, three-way mutual information, application of partial information decomposition to the problem, and coherent infomax. Nevertheless, please feel free to not pick me for reviewing this submission in favor of another one as you see fit. Thanks.
@schmidDan you are super picked — thank you so much for the info. 😊
@oliviaguest Thanks a lot! Then I'm more than happy to aid as a reviewer. 🙂
Excellent! I am very happy to see that you have taken charge of the reviewing process Olivia and Daniel.
I'll see what I can do to improve figure 2 as Nicolas suggested by tomorrow.
@sepehrmn I'm the editor. I'll find you a 2nd reviewer soon hopefully. Any suggestions welcome, of course!
Thanks for providing some preliminary feedback. Figure 2 has now been adjusted accordingly.
Perhaps due to the current crisis I am struggling a bit to find a 2nd reviewer as quickly as I would like. @sepehrmn if you have any ideas, they are welcomed. I will keep looking of course.
👋 Hi @pwollstadt @Abzinger @pmediano @finnconor — would any one of you be interested in reviewing this? 😊
@oliviaguest I’m interested … :)
@oliviaguest I'm interested.
I just want to note that I'm currently working with Sepher in the same lab. But I haven't been part of the work he is submitting (no involvement in the discussion, the techniques used, the implementation of the code, the results obtained, or the written manuscript). As a reference, I suggest looking at my ORCID
OK, fantastic. I suggest you both (@reubsjw @Abzinger) act as 2nd and 3rd reviewers. The rest of the people tagged above can have a chance (if they so wish) to review in future. Great!
If all three @reubsjw @Abzinger @schmidDan can give me a rough ETA for your reviews that would be very helpful. Please also, if not already, take a look at: http://rescience.github.io/edit/
I’ll aim for tomorrow or Saturday if that’s ok?
The author replicates the target article's findings by implementing the same mathematical models in a modern computational environment. The target article describes and demonstrates, in information-theoretical terms and using computational simulation, additive and modulatory relationships (seperately and jointly) of receptive and contextual inputs to a hypothetical neuron, with that neuron's output, in terms of the information transmitted.
My only major concern is the commenting in the code, which includes, e.g. "to-do's" (presumably as an aide memoir to the author). The commenting should also, where possible, reference the numbered equations in the article, but I would also encourage them to provide short explanations for each block/loop etc of the code (which they have already done nicely in many places), to help readers orient themselves.
It would also be nice if the statement that the original paper is "...quantitatively replicated..." was qualified a little more. I note that Figure 1, for example, appears to be noticably different (albeit the qualitative shapes of the lines are the same, and the layout of the figure in Smyth et al is harder to read than in the present replication). Also, speaking of figures being easier or harder to read, I'd like to recommend that for figure 2, the author use the same "perspective" (as it were) on the 3D plots as is used in the Smyth article, since that layout is much more readable in my opinion.
@oliviaguest ETA would be by the end of today or tomorrow for my review.
@oliviaguest At the latest, I will have my review done by the end of the day
Gosh, you're all super fast. Thank you. 😱 😍
@sepehrmn I had a fun time reading and reviewing your code and manuscript. Nice job! Please find my review below.
My review is based on commit 2c117cf.
README.md
telling the reader where to enter a new activation function within the code.P(r)
and P(c)
were set to 0.5
as mentioned by the authors of the original article.P(C=1|R=1)
was set to `0.889972
. As the original article, you could also describe why this was done in order to make for a more self-contained read.instance
of the probability distributions, i.e. their binary values, while you are using them within your text to refer to the distributions themselves. Further, non-bold "r" and "c" seem to be used for continuous values (Eqs.5-8) and binary values (Eqs.9-11) alike throughout your manuscript, while the original publication used different notations. This makes it a little bit harder to follow the text knowing as well the original work.[0,10]
.README.md:4
- should be Smyth
instead of smyth
?pip install -r requirements.txt
README.md
though, pretty obvious, one could additionally specify from which directory to run the code (since README.md exists on two directory levels)analysis.py::cal_mis
.main.py:43
- #TODO
shouldn't probably go into published code version. Furthermore, it would be probably "cleaner" to have the assignment on the level of the calling function, similar to the assignments for calc_X__R
and calc_X__C
main.py:78-82
- usage of S
array-protocol type string is not recommended according to the numpy docsmain.py:119
- commented out code - I guess, it's a left-over?main.py:120,131
- why is for the same functionality (computation of exp
) once the np
and once the math
implementation used?main.py:150
- comment should probably be # P(x|r,c)
instead of # P(x,r,c)
?main.py:186
- while seemingly working fine as is, wouldn't it be sensible to base plotting on the analytical_results
' r
entries, which have been used for computing the respective value
s, rather than params.r_magnitudes
(from which rmag
was derived)? Same goes for main.py:187
and main.py:304
.main.py:190
- where is the specified range coming from? It seemingly doesn't match the original publication, e.g. there are only 4 sample sizes smaller than sample size of 200, while in the original publications there are 5 and their largest sampling size is smaller than 1000, while yours is bigger than that.main.py:213,237
- usage of random number generator: I would suggest to specify a seed value for the rng at the beginning of the script in order to increase reproducibility.main.py:249
- this X
assignment seems to never be used and is overwritten in main.py:273
np.finfo(float).eps
as it was done for e.g. main.py:278
as well to the computations in e.g. main.py:24,33,55,68
? And similarly treat the math.log
calls in e.g. analysis.py:20,23,26
?main.py:300-302
- if not giving a requirements.txt
specifying the version of the numpy
package, one might want to explicitly specify the ddof
argument of numpy.std
in order to not be reliant on the default value implementation across numpy versions (although they probably won't change)plotting.py:32
- the label for the x-axis set here seems to be different from what is found in the article's Fig.1. How comes?plotting.py:74
- why is a transpose (T
) necessary here? and should reshaping not happen according to Y.shape[0]
as well instead of (X.shape[0]
,X.shape[0]
)?plotting.py:77
- the view of the 3D plot should be adjusted to more faithfully capture the original publication's Fig.2. Not sure, whether it's due to the angle of viewing, or the scaling of the z-axis.plotting.py:86
- commented out code line hereplotting.py:88,91,94
- comments don't seem to accuractely reflect the functions' arguments anymoreplotting.py:plot_fig3
- commented out lines of code throughout the functionplotting.py:104-105
- rather than documenting what is removed, it would be more instructive to document what is kept from analytical_results
plotting.py:120
- limits seem slightly different from the ones in the original paper's Fig.3(edit: added link to commit I based my review on)
@sepehrmn It was enjoyable going through the manuscript and the implementation. Below is my review which is hopefully useful to improve the work.
Here are some suggestions to improve the manuscript and the code.
As a general comment, clean the code of any internal comments like #ToDo or something code that you commented out. Below are two main issues that I think need improving.
main.py
, I noticed that the case of p(Y=y)=0 in P(X=x|Y=y) is either ignored or dealt with in an Adhoc manner. I suggest that when adding an if statement to declare P(X=x|Y=y) = 0 when P(Y=y) = 0. main.py:190
, the number range should be np.arange(50, 1050, 50)
instead of np.arange(40, 1040, 40)
. This might be one of the reasons that Figure 3 was not replicated quantitatively accurately. Also, defining a seed for a random sampling is very helpful for you to debug and others to reproduce an exact replica of your figure.I thank the reviewers for their comments on the code and manuscript and for helping me improve the publication. I would like to say thanks again for their fast and meticulous reviews.
I hope to adjust and improve the submission to the reviewers' satisfaction by Tuesday.
I will write a series of comments in response to the reviewers today. Please wait until Tuesday (tomorrow) to read and respond as there will be changes happening.
Thank you.
@reubsjw many thanks for your work and review. I have adjusted the code and will update the manuscript shortly. Below I explain how I've made adjustments.
My only major concern is the commenting in the code, which includes, e.g. "to-do's" (presumably as an aide memoir to the author). The commenting should also, where possible, reference the numbered equations in the article, but I would also encourage them to provide short explanations for each block/loop etc of the code (which they have already done nicely in many places), to help readers orient themselves.
It would also be nice if the statement that the original paper is "...quantitatively replicated..." was qualified a little more. I note that Figure 1, for example, appears to be noticably different (albeit the qualitative shapes of the lines are the same, and the layout of the figure in Smyth et al is harder to read than in the present replication). Also, speaking of figures being easier or harder to read, I'd like to recommend that for figure 2, the author use the same "perspective" (as it were) on the 3D plots as is used in the Smyth article, since that layout is much more readable in my opinion.
@schmidDan much appreciated. I'll explain how I've addressed some issues here. I'll explain the rest soon and alert when everything has been pushed.
- 'Motivation' - the second claim is correct, though it would be nice to have a respective instruction in e.g.
README.md
telling the reader where to enter a new activation function within the code.
Done. Added in the readme in the code directory.
- 'Reproduction' - I'm pretty sure this section should be called 'Replication' instead of 'Reproduction' (cf. Rescience's FAQ). The author achieved a full replication of the original work. A claim is made to have 'qualitatively' and 'quantitatively' reproduced the original results by having reproduced all three figures of the original paper.
I have changed from "reproduction" to "replication". I note the minor quantitative differences in the manuscript.
- Fig.2: Either the aspect ratio or the view point don't fit quite well with the original paper's Fig.2, hindering interpretability quite a bit. While you match the original article's notation by writing "0.889 972" I would favor "0.889972", so that the notation is consistent throughout your article (cf. how you formatted the number on page 1). (See as well my remarks to the implementation.)
I have changed the layout and perspective of figure 2. I have removed the space in "0.889972"
- Fig.3: The limits of the y-axes and the chosen sample sizes seem to mismatch the ones of the original publication making a judgment about replication more difficult. While the authors of the original paper didn't put this note in their caption either, I believe it would make for a more comprehensive read to put a note into Fig.3's caption stating that these plots refer to the 'modulatory' activation function. (See as well my remark sto the implementation.)
Limits have been adjusted. I include that this figure is with the modulatory activation function.
- The notion of bold "r" and bold "c" were reserved in the original publication for an
instance
of the probability distributions, i.e. their binary values, while you are using them within your text to refer to the distributions themselves. Further, non-bold "r" and "c" seem to be used for continuous values (Eqs.5-8) and binary values (Eqs.9-11) alike throughout your manuscript, while the original publication used different notations. This makes it a little bit harder to follow the text knowing as well the original work.
I removed the bold version of r and c.
- 'Information surface plots for different input weights' You claim that "c remains constant at 1". I believe this is not the case, since it was varied as well over the range of
[0,10]
.
Yes, indeed. Fixed.
README.md:4
- should beSmyth
instead ofsmyth
?
Yes, adjusted.
- Code/README.md - duplicate with README.md
I still keep 2 readme files, but the content has been adjusted.
- Requirements.txt would be nice to have in order to run e.g.
pip install -r requirements.txt
Done.
README.md
though, pretty obvious, one could additionally specify from which directory to run the code (since README.md exists on two directory levels)
The readme not in the code directory is different now.
- Docstrings for functions would help for an easier read of e.g.
analysis.py::cal_mis
.
Done. I added more comments and docstrings for analysis.py and plotting.py. The functions in main.py are self-explanatory.
main.py:43
-#TODO
shouldn't probably go into published code version. Furthermore, it would be probably "cleaner" to have the assignment on the level of the calling function, similar to the assignments forcalc_X__R
andcalc_X__C
Cleaned the code more in main.py.
main.py:78-82
- usage ofS
array-protocol type string is not recommended according to the numpy docs
Changed to Object.
main.py:119
- commented out code - I guess, it's a left-over?
Removed.
main.py:120,131
- why is for the same functionality (computation ofexp
) once thenp
and once themath
implementation used?
Changed all to one to remain consistent.
main.py:150
- comment should probably be# P(x|r,c)
instead of# P(x,r,c)
?
Changed.
main.py:190
- where is the specified range coming from? It seemingly doesn't match the original publication, e.g. there are only 4 sample sizes smaller than sample size of 200, while in the original publications there are 5 and their largest sampling size is smaller than 1000, while yours is bigger than that.
adjusted the range.
main.py:213,237
- usage of random number generator: I would suggest to specify a seed value for the rng at the beginning of the script in order to increase reproducibility.
Using a seed now.
main.py:249
- thisX
assignment seems to never be used and is overwritten inmain.py:273
Removed.
main.py:300-302
- if not giving arequirements.txt
specifying the version of thenumpy
package, one might want to explicitly specify theddof
argument ofnumpy.std
in order to not be reliant on the default value implementation across numpy versions (although they probably won't change)
requirements.txt is provided.
plotting.py:32
- the label for the x-axis set here seems to be different from what is found in the article's Fig.1. How comes?
Adjusted.
plotting.py:77
- the view of the 3D plot should be adjusted to more faithfully capture the original publication's Fig.2. Not sure, whether it's due to the angle of viewing, or the scaling of the z-axis.
Layout and perspective changed.
plotting.py:86
- commented out code line here
Removed.
plotting.py:88,91,94
- comments don't seem to accuractely reflect the functions' arguments anymore
Adjusted.
plotting.py:plot_fig3
- commented out lines of code throughout the function
Cleaned.
plotting.py:104-105
- rather than documenting what is removed, it would be more instructive to document what is kept fromanalytical_results
Done.
plotting.py:120
- limits seem slightly different from the ones in the original paper's Fig.3
I've adjusted the y axes.
@Abzinger Thank you again for your review and comments on how to improve this submission. I'll explain how I've addressed some issues here. I'll explain the rest soon and alert when everything has been pushed.
- In Background I would replace the determiner their by inputs for clarity.
I have improved the manuscript in this section.
- In Methods -- Input I would replace probability distributions for r and c were created... by The probability distributions of sampling r and c are defined as in the original article. They are as follows [equations] where P(C=1|R=1) = 0.889972
I removed r and c in this part of the manuscript for another reviewer due to another matter.
- In Methods -- Information theory, I would replace and write it as the equation below. by is defined as follows. The same follows for the rest of the equations in this subsection.
Done.
- In Results -- Activation functions and transmitted information, you should mention how many samples you were used. The aspect ratio of Figure 1 should match the original paper
For figures 1 and 2, the MI results are generated by directly entering the actual precise probability distributions in equations 9-11. Simulations of a single binary neuron, generation of r and c vectors and probability distributions based on those vectors is only done for figure 3. There is nothing suggesting this is not how it was done in the original paper as well. In fact, it is indeed the whole point of figure 3. They use figures 1-2 to study contextual modulation and it does make sense to do those on 'ground truths' then study how many observations is needed in a simulation/experiment to get to those as in figure 3.
I have changed it a bit. I understand the perspective and aspect ratio of figure 2 is important because it should show all the data points, but as figure 1 is 2D, it is still possible to view all the data but it would not look good with the current number of data points if the width was reduced compared to the height more than this.
- In Results -- Information surface plots for different input weights, the following claim about c is not correct while c remains constant at 1 for figure 2. The value of c is also variable otherwise there is no need for a 'c' axis.
Yes. Fixed.
- In Results -- Information surface plots for different input weights, Figure 2 is qualitatively similar to the original one but not quantitatively. For example, figures (a) and (c) peak at 0.48 while in the original paper they peak at 0.4.
In the original paper 0.4 is the higher text on the z axis, but there are ticks above it and the data extends above 0.4. Nonetheless, I note the minor quantitative differences in the manuscript.
- In Results -- Information surface plots for different input weights, in Figure 2 the aspect ratio and the angle should match that of the original figure. Also, the heat map should be added to the plots and there is an unnecessary space for in 0.889972 which should be removed.
- In Results -- Sampling biases and variances, in Figure 3 the error bars for I(X;R;C) don’t match the original one at least for the 40 sample case. This makes the average divert for a smaller number of samples but it converges eventually to the same value as the number of samples grows. However, there is nothing to be done from Sepher's side unless he is able to obtain the random seed at which the original paper data was generated.
I'm using a seed now.
- In Discussion, In the same way can be replaced by Similarly.
Done.
- In Discussions, I suggest replacing the same figures here can be used study the different activation functions and contextual amplification. by _The information-theoretical analysis used here can be employed to other activation functions and contextual amplification.
It was meant that they could use the figures in the replication to draw the same conclusions about contextual modulation as they did in the original paper but I have made the requested changes and will adapt accordingly.
As a general comment, clean the code of any internal comments like #ToDo or something code that you commented out. Below are two main issues that I think need improving.
Code has been cleaned more.
- In
main.py:190
, the number range should benp.arange(50, 1050, 50)
instead ofnp.arange(40, 1040, 40)
. This might be one of the reasons that Figure 3 was not replicated quantitatively accurately. Also, defining a seed for a random sampling is very helpful for you to debug and others to reproduce an exact replica of your figure.
@schmidDan please find my answers to the rest of your concerns below:
- Fig.1: Each curve seemingly contains more samples than the ones of the original paper. I think this is a plus. What makes it harder to compare the original Fig.1 with the one of the author is the different aspect ratio of the figures. The author's figure seems to be stretched w.r.t. its width in comparison to the original one.
Adjusted a bit and put justification in the manuscript.
- You wrote that
P(C=1|R=1)
was set to`0.889972
. As the original article, you could also describe why this was done in order to make for a more self-contained read.
Done.
- You state 'If there is modulatory amplification as in (6), I(X;R;C) should increase more rapidly as a function of R. This is evident from Figure 1 (a) where the ”zero context” function [...]'. Should this rather be "as a function of C", since you are making your point by referring to the absence of context in the following sentence?
I've rewritten discussion section based on the feedback by the reviewers.
main.py:186
- while seemingly working fine as is, wouldn't it be sensible to base plotting on theanalytical_results
'r
entries, which have been used for computing the respectivevalue
s, rather thanparams.r_magnitudes
(from whichrmag
was derived)? Same goes formain.py:187
andmain.py:304
..plotting.py:74
- why is a transpose (T
) necessary here? and should reshaping not happen according toY.shape[0]
as well instead of (X.shape[0]
,X.shape[0]
)?
good idea. Changed.
transpose is no longer needed. The shapes were equal but the code is different now.
- Avoiding divisions by zero: Would it make sense to add
np.finfo(float).eps
as it was done for e.g.main.py:278
as well to the computations in e.g.main.py:24,33,55,68
? And similarly treat themath.log
calls in e.g.analysis.py:20,23,26
?
If the denominator is 0, I set them to 0. For analysis.py, eps was already added at the top during assignment of the denominators to variables.
@Abzinger much appreciated again. I address your remaining concerns here:
- In Discussion, In the same way can be replaced by Similarly.
- In Discussions, I suggest replacing the same figures here can be used study the different activation functions and contextual amplification. by The information-theoretical analysis used here can be employed to other activation functions and contextual amplification.
- In Discussions, I suggest replacing Here are some examples: by Henceforth, we present two demonstrations of such usage one for the activation function and the other for the contextual amplification. This makes the sentence more informative and leads the path to the reader.
- In Discussions, If there is modulatory amplification as in (6), Did you mean if you change the 0.5 parameter or add some constant in the exponential? This sentence should be clearer. Maybe give an example.
- In Discussions, Another example, This is an example for a different contextual amplification. So, I would write an introductory sentence stating this.
- In Discussions, In conclusion, the main results of their work is the verb should be 'are' instead of 'is'.
I've rewritten the discussion section based on the feedback from the reviewers.
- In
main.py
, I noticed that the case of p(Y=y)=0 in P(X=x|Y=y) is either ignored or dealt with in an Adhoc manner. I suggest that when adding an if statement to declare P(X=x|Y=y) = 0 when P(Y=y) = 0.
Done. If denominator is 0, I set it to 0.
@reubsjw @schmidDan @Abzinger the process is now complete from my side.
Please proceed.
@sepehrmn thanks for addressing my concerns and sorry for the delay. Please find my response below.
article/context.tex:17
- While it's good to not have the confusing association bold/non-bold r
and c
anymore, now there's the problem of not having the notation of Eqs. 5ff. explained at all w.r.t. r
and c
. Please meantion their meaning, e.g. in article/context.tex:37
(subsection "Activation Fucntions").article/context.tex:86
- "Since the exact tick sizes for the x axis is not written [...]" - should probably be either "tick size [...] is" or "tick sizes [...] are". Furthermore, I guess, the issue is rather that the original paper didn't give the "step size" for evaluating the functions than that they didn't give the "tick size".article/context.tex:86
- "[...] but rules out missing the higher maximum value due to not having that particular data point". That would be as well my take here. Given the densely sampled functions in your Fig.1, the qunatitative difference doesn't seem to stem from using different input values for the evaluation than the original paper did. So far I'm fine with the statement. Which is left implicitly/up to the reader, is the question of "Where are the differences stemming from then, if not from the used input values?". You could refer again to the argument you give as an explanation in article/context.tex:11
(section "Replication"). Other than that, I didn't came up with an idea either for how to explain the differences here. Stilistically, I would maybe exchange the "[...] but rules out [...]" for a "[...] in order to rule out the possibility of [...]" to better motivate the choice of the dense sampling.article/context.tex:95
- The new sentence is tautology "There are no notable quantitative differences and the values look very similar."article/context.tex:107
- "[...] this can be attributed to use of a different seed value and their programming language". Not too sure about the statement w.r.t. the programming language - it is probably a complex of the bit architectures back then, library support for languages w.r.t. to scientific computing/numerical accuracy etc. than simply the language chosen.article/context.tex:119
- "In order to serve as a self-contained manuscript, [...] included [...]" I would skip that justification part of the sentence. As soon as the manuscript is accepted it is by definition self-contained by meeting the Rescience C standards.article/context.tex:119
- "[...] shows that Increasing C alone [...]" Typo here: should be "increasing" instead of "Increasing".[0.0, 0.1, ..., 0.5]
instead of [0.0, 0.2, 0.4]
.code/README.md:31
- "The fastest way to test your activation function [...]" would probably be better as "The fastest way to implement and investigate a [new/custom] activation function [...]".code/main.py:23-26,35-38,49-52,65-68,290-294
- I see that you now try to consistently adress division-by-zero issues. Nevertheless, why would division by zero lead to an outcome of "0.0" and not e.g. "numpy.inf"?code/plotting.py:17
- Why is the unique
necessary/benefitial here? What if the Y
s have been generated by non-unique X
s? Wouldn't this mean, that one would get confused X,Y
pairings then? Nevertheless, for the current status of the code base with having unqiue X
s this should be fine as is.@sepehrmn Thanks for addressing the comments and the current version is ready for publishing.
@schmidDan much appreciated. Here is I have addressed the remaining concerns:
- In
article/context.tex:17
- While it's good to not have the confusing association bold/non-boldr
andc
anymore, now there's the problem of not having the notation of Eqs. 5ff. explained at all w.r.t.r
andc
. Please meantion their meaning, e.g. inarticle/context.tex:37
(subsection "Activation Fucntions").
Added "Where r is an instance of R, and c an instance of C, multiplied by the weight of the connection."
- In
article/context.tex:86
- "Since the exact tick sizes for the x axis is not written [...]" - should probably be either "tick size [...] is" or "tick sizes [...] are". Furthermore, I guess, the issue is rather that the original paper didn't give the "step size" for evaluating the functions than that they didn't give the "tick size".- In
article/context.tex:86
- "[...] but rules out missing the higher maximum value due to not having that particular data point". That would be as well my take here. Given the densely sampled functions in your Fig.1, the qunatitative difference doesn't seem to stem from using different input values for the evaluation than the original paper did. So far I'm fine with the statement. Which is left implicitly/up to the reader, is the question of "Where are the differences stemming from then, if not from the used input values?". You could refer again to the argument you give as an explanation inarticle/context.tex:11
(section "Replication"). Other than that, I didn't came up with an idea either for how to explain the differences here. Stilistically, I would maybe exchange the "[...] but rules out [...]" for a "[...] in order to rule out the possibility of [...]" to better motivate the choice of the dense sampling.
Changed to "In order to rule out the possibility of missing the higher maximum value as a result of not having that particular data point, a small step size of 0.1 was used for each data point which leads to a higher number of data points on this figure than the original publicationʼs."
- In
article/context.tex:95
- The new sentence is tautology "There are no notable quantitative differences and the values look very similar."
Changed to "There are no notable quantitative differences."
- In
article/context.tex:107
- "[...] this can be attributed to use of a different seed value and their programming language". Not too sure about the statement w.r.t. the programming language - it is probably a complex of the bit architectures back then, library support for languages w.r.t. to scientific computing/numerical accuracy etc. than simply the language chosen.
Changed to "programming environment"
- In
article/context.tex:119
- "In order to serve as a self-contained manuscript, [...] included [...]" I would skip that justification part of the sentence. As soon as the manuscript is accepted it is by definition self-contained by meeting the Rescience C standards.
Removed.
- In
article/context.tex:119
- "[...] shows that Increasing C alone [...]" Typo here: should be "increasing" instead of "Increasing".
Done.
- Figure 1: W.r.t. the yticks of figure 1 it would be best to match the ones used in the original paper for having an easier comparison. E.g. Fig.1 (a) would be
[0.0, 0.1, ..., 0.5]
instead of[0.0, 0.2, 0.4]
.
Done.
- In
code/README.md:31
- "The fastest way to test your activation function [...]" would probably be better as "The fastest way to implement and investigate a [new/custom] activation function [...]".
Done.
- In
code/main.py:23-26,35-38,49-52,65-68,290-294
- I see that you now try to consistently adress division-by-zero issues. Nevertheless, why would division by zero lead to an outcome of "0.0" and not e.g. "numpy.inf"?
This was recommended by another reviewer. None of these cases actually happen during runtime, i.e., the denominator is never 0 and with the seed it is deterministic.
- In
code/plotting.py:17
- Why is theunique
necessary/benefitial here? What if theY
s have been generated by non-uniqueX
s? Wouldn't this mean, that one would get confusedX,Y
pairings then? Nevertheless, for the current status of the code base with having unqiueX
s this should be fine as is.
I use np.unique(results_zero_c['r'])
because results_zero_c
has information about same r value for different information theoretic metrics and activation functions. E.g., it contains multiple repetitions of 0.0 and 0.1.
I am satisfied that the article is now ready for publication. With apologies for my relative quietness lately, I'd like to commend the author for their response to my and the other reviewer's comments.
Fabulous work, everybody! Massive thank you to all the work put in by the reviewers @reubsjw @Abzinger @schmidDan — really high quality feedback. 👏
@sepehrmn can you update the PDF with the final details so we can get this published, please?
@oliviaguest many thanks!
I've added the reviewers and editor and a few minor things. Both the code and article are in the same repository. I have to now make DOI for them on Zenodo and we are done?
I actually am not sure if you are meant to do that or me — I think it's me. But I'll tag @rougier just in case. The instructions are here but I think it's all for me to do now, so don't worry at all: https://github.com/ReScience/articles/blob/master/README.md
I'll post back here when I upload it and it's all ready to go. Have you taken one last check of the proofs and are happy with them? 😊
@reubsjw could you please confirm that your information as it appears on the PDF is correct? The other two reviewers put their ORCID but I searched for yours.
We're now using software heritage for savign the code and the procedure is even easier than Zendodo. Just look at https://www.softwareheritage.org/save-and-reference-research-software/.
You should obtain a swid that can be included in the PDF (have a look a the new article template)
@reubsjw could you please confirm that your information as it appears on the PDF is correct? The other two reviewers put their ORCID but I searched for yours.
Thanks for checking. My ORCID is 0000-0002-5592-7562. Please include my middle initial (i.e. Reubs J Walsh)
The pdf is fine.
I tried to submit to software heritage. It says "succeed" but then says no snapshot is available.
I've resubmitted one more time which should be done in a few hours.
@sepehrmn I set up a PR https://github.com/sepehrmn/mahmoudian-2020-rescience/pull/1 to update the metadata and if you can recompile with a publication of 9th June, I'l publish it immediately. 😊
@oliviaguest Many thanks!
I don't see the Zenodo link maybe it's not public yet, but the pdf is done now with the provided information!
@rougier do you know what I'm doing wrong with the publish.py
script? I'll keep trying, but any help is greatly appreciated.
(base) MBpro:articles olivia$ ./publish.py --sandbox --metadata ../mahmoudian-2020-rescience/article/metadata.yaml --pdf ../mahmoudian-2020-rescience/article/Reproduction_of_Smyth_et_al__1996.pdf
Uploading content to Zenodo... Traceback (most recent call last):
File "./publish.py", line 204, in <module>
upload_content(server, token, article_id, article_file)
File "./publish.py", line 25, in upload_content
raise IOError("%s: " % response.status_code + response.json()["message"])
OSError: 404: PID does not exist.
@rougier Did I do this correctly? https://zenodo.org/record/3885793
Just in cased needed I tried to just publish it and I think something went wrong but not with the paper? I'm so confused by this process every time! Sorry!
(base) MBpro:articles olivia$ ./publish.py --zenodo --metadata ../mahmoudian-2020-rescience/article/metadata.yaml --pdf ../mahmoudian-2020-rescience/article/Reproduction_of_Smyth_et_al__1996.pdf
Uploading content to Zenodo... zenodo.org
done!
Updating metadata to Zenodo... done!
Publishing on Zenodo... done!
Entry is online at https://zenodo.org/record/3885793
Creating local directory...
Saved working directory and index state WIP on master: 30db7cb minor
Switched to a new branch '10.5281_zenodo.3885793'
Traceback (most recent call last):
File "./yaml-to-bibtex.py", line 62, in <module>
content = generate_bibtex(filename_in, article)
File "./yaml-to-bibtex.py", line 28, in generate_bibtex
"}}".format(filename=filename, _=article))
AttributeError: 'Repository' object has no attribute 'swh'
fatal: pathspec '10.5281_zenodo.3885793/article.bib' did not match any files
[10.5281_zenodo.3885793 5dee927] Added entry 10.5281/zenodo.3885793
2 files changed, 119 insertions(+)
create mode 100644 10.5281_zenodo.3885793/article.pdf
create mode 100644 10.5281_zenodo.3885793/article.yaml
On branch 10.5281_zenodo.3885793
Changes not staged for commit:
(use "git add <file>..." to update what will be committed)
(use "git checkout -- <file>..." to discard changes in working directory)
modified: publish.py
Untracked files:
(use "git add <file>..." to include in what will be committed)
__pycache__/
no changes added to commit (use "git add" and/or "git commit -a")
Dropped refs/stash@{0} (d73158cad1acba14db99f6e68b18374ae2bedbd8)
M publish.py
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
---------------------------------------------------
You can now merge 10.5281_zenodo.3885793 into master
---------------------------------------------------
(base) MBpro:articles olivia$ ./publish.py --zenodo --metadata ../mahmoudian-2020-rescience/article/metadata.yaml --pdf ../mahmoudian-2020-rescience/article/Reproduction_of_Smyth_et_al__1996.pdf
Uploading content to Zenodo... Traceback (most recent call last):
File "./publish.py", line 204, in <module>
upload_content(server, token, article_id, article_file)
File "./publish.py", line 25, in upload_content
raise IOError("%s: " % response.status_code + response.json()["message"])
OSError: 400: Filename already exists.
(base) MBpro:articles olivia$ git push
Warning: untrusted X11 forwarding setup failed: xauth key data not generated
To github.com:ReScience/articles.git
! [rejected] master -> master (fetch first)
error: failed to push some refs to 'git@github.com:ReScience/articles.git'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
(base) MBpro:articles olivia$ git pull
Warning: untrusted X11 forwarding setup failed: xauth key data not generated
remote: Enumerating objects: 10, done.
remote: Counting objects: 100% (10/10), done.
remote: Compressing objects: 100% (7/7), done.
remote: Total 9 (delta 2), reused 9 (delta 2), pack-reused 0
Unpacking objects: 100% (9/9), done.
From github.com:ReScience/articles
30db7cb..39bf30e master -> origin/master
Updating 30db7cb..39bf30e
Fast-forward
0.5281_zenodo.3886412/article.bib | 22 ++++++++
0.5281_zenodo.3886412/article.pdf | Bin 0 -> 270421 bytes
0.5281_zenodo.3886412/article.yaml | 112 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 134 insertions(+)
create mode 100644 0.5281_zenodo.3886412/article.bib
create mode 100644 0.5281_zenodo.3886412/article.pdf
create mode 100644 0.5281_zenodo.3886412/article.yaml
(base) MBpro:articles olivia$ git push
Warning: untrusted X11 forwarding setup failed: xauth key data not generated
Everything up-to-date
(base) MBpro:articles olivia$ ls
0.5281_zenodo.3886412 10.5281_zenodo.3162890 10_5281_zenodo.3538217
10.5072_zenodo.416801 10.5281_zenodo.3234524 README.md
10.5281_zenodo.3069619 10.5281_zenodo.3528175 __pycache__
10.5281_zenodo.3158244 10.5281_zenodo.3538217 article.py
10.5281_zenodo.3160540 10.5281_zenodo.3630224 process.py
10.5281_zenodo.3161734 10.5281_zenodo.3763416 publish.py
10.5281_zenodo.3162114 10.5281_zenodo.3842360 yaml-to-bibtex.py
(base) MBpro:articles olivia$
I think the paper is online but I received 3 notifications from Zenodo (for the ReScience collection), I need to check. After publication, a directory with the DOI of the paper is created and the article + metadata is stored. Then the script makes a git push .
Original article: Smyth, D., Phillips, W. A. and Kay, J.(1996) 'Measures for investigating the contextual modulation of information transmission', Network: Computation in Neural Systems, 7:2,307 — 316
PDF URL: https://github.com/sepehrmn/mahmoudian-2020-rescience/blob/master/article/Reproduction_of_Smyth_et_al__1996.pdf
Metadata URL: https://github.com/sepehrmn/mahmoudian-2020-rescience/blob/master/article/metadata.yaml
Code URL: https://github.com/sepehrmn/mahmoudian-2020-rescience/tree/master/code
Scientific domain: Computational Neuroscience
Programming language: Python
Suggested editor: