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

[Re] Measures for investigating the contextual modulation of information transmission. #29

Closed
sepehrmn opened this issue Apr 18, 2020 · 80 comments

Comments

@sepehrmn
Copy link

sepehrmn commented Apr 18, 2020

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:

@sepehrmn
Copy link
Author

sepehrmn commented Apr 20, 2020

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.

@rougier
Copy link
Member

rougier commented May 3, 2020

@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 ?

@oliviaguest
Copy link
Member

Happy to take this on. 😄

@schmidDan
Copy link

@oliviaguest If you don't mind, I'd be happy to act as a reviewer for this submission. Would be my first one then. 🙂

@rougier
Copy link
Member

rougier commented May 5, 2020

@oliviaguest Thank you @schmidDan Thank you, I guess you can start your review but @oliviaguest will confirm it.

@oliviaguest
Copy link
Member

@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.

@schmidDan
Copy link

@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.

@oliviaguest
Copy link
Member

@schmidDan you are super picked — thank you so much for the info. 😊

@schmidDan
Copy link

@oliviaguest Thanks a lot! Then I'm more than happy to aid as a reviewer. 🙂

@sepehrmn
Copy link
Author

sepehrmn commented May 5, 2020

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.

@oliviaguest
Copy link
Member

oliviaguest commented May 5, 2020

@sepehrmn I'm the editor. I'll find you a 2nd reviewer soon hopefully. Any suggestions welcome, of course!

@sepehrmn
Copy link
Author

sepehrmn commented May 6, 2020

Thanks for providing some preliminary feedback. Figure 2 has now been adjusted accordingly.

@oliviaguest
Copy link
Member

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.

@oliviaguest
Copy link
Member

👋 Hi @pwollstadt @Abzinger @pmediano @finnconor — would any one of you be interested in reviewing this? 😊

@reubsjw
Copy link

reubsjw commented May 7, 2020

@oliviaguest I’m interested … :)

@Abzinger
Copy link

Abzinger commented May 7, 2020

@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

@oliviaguest
Copy link
Member

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/

@reubsjw
Copy link

reubsjw commented May 7, 2020

I’ll aim for tomorrow or Saturday if that’s ok?

@reubsjw
Copy link

reubsjw commented May 7, 2020

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.

@schmidDan
Copy link

@oliviaguest ETA would be by the end of today or tomorrow for my review.

@Abzinger
Copy link

Abzinger commented May 8, 2020

@oliviaguest At the latest, I will have my review done by the end of the day

@oliviaguest
Copy link
Member

Gosh, you're all super fast. Thank you. 😱 😍

@schmidDan
Copy link

schmidDan commented May 8, 2020

@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.

Review Summary

  • Full replication | Partial replication | Failed replication | Reproduction: The author conducted a successful, 'full replication' of the original work.
  • Licensing: An MIT License is provided.
  • Reproducibility of the replication: I was able to perform a reproduction without any adaptations to the code. While using a different operating system, I followed all other instructions provided by the author for reproduction.
  • Clarity of the code: Overall clean, understanable implementation. See my 'Remarks w.r.t. the implementation' for potential improvements still.
  • Clarity and completeness of the accompanying article: The accompanying article makes for a compact, self-contained read capturing the main methodological points and theoretical findings of the original work. Obstacles of the author performing the replication so far been have only adressed implicitly (see their 'Acknowledgments' section). For further details w.r.t the article see my 'Remarks w.r.t. the manuscript'.

Remarks w.r.t. the manuscript

  • '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.
  • '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. In general, I'm supportive w.r.t. the author's statement, although having four remarks:
    • Overall the results match quite nicely the findings of the original publication, definitely being worth the term of a 'qualitative' replication. Concerning the term 'qunatitative' it's a little bit harder to judge, since, to my knowledge, no dataset has been released accompanying the original publication. Thus, the 'quantitative' comparison is based on visually comparing likely formatted figures, which makes the term 'quantitative' in this case at least debatable. Given the
    • 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.
    • 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.)
    • 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.)
  • 'Methods'
    • You could have provided as well the information that P(r) and P(c) were set to 0.5 as mentioned by the authors of the original article.
    • 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.
    • 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.
    • '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].
  • 'Discussion'
    • 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 found no typos, cheers!

Remarks w.r.t. the implementation

  • README.md:4 - should be Smyth instead of smyth?
  • Code/README.md - duplicate with README.md
  • Requirements.txt would be nice to have in order to run e.g. 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)
  • Docstrings for functions would help for an easier read of e.g. 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 docs
  • main.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 values, 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
  • 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 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 here
  • plotting.py:88,91,94 - comments don't seem to accuractely reflect the functions' arguments anymore
  • plotting.py:plot_fig3 - commented out lines of code throughout the function
  • plotting.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)

@Abzinger
Copy link

Abzinger commented May 8, 2020

@sepehrmn It was enjoyable going through the manuscript and the implementation. Below is my review which is hopefully useful to improve the work.

Overview

  • The actual replication of the research. The author has conducted a successful full replication of the work. Minor improvements are needed and the author can look them up in the comments section below.
  • Reproducibility of the replication. I was able to reproduce the results using the authors with no problems.
  • Clarity of the code and the instructions for running it. The code is well written and the instructions are sufficient. Please check the comments section below for minor improvements.
  • Clarity and completeness of the accompanying article. In general, the manuscript is clear, self-contained, and well written. Some minor suggestions regarding the language and the clarity are reported in the comments section below.

Comments

Here are some suggestions to improve the manuscript and the code.

Manuscript

  • In Background I would replace the determiner their by inputs for clarity.
  • 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
  • In Methods -- Input, I would mention the reason for choosing P(C=1|R=1) = 0.889972 since it is relevant to the results obtained.
  • 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.
  • 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
  • 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.
  • 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 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.
  • 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'.

Implementation

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.

  • 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.
  • In 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.

@rougier
Copy link
Member

rougier commented Jun 9, 2020

I'm not sure about the X11 forxwarding part. Best would be to clone the repo into another place and check if they're the same (make lasy commit).

@khinsen
Copy link

khinsen commented Jun 9, 2020

  1. It's me who published two papers this morning, so the three messages from Zenodo are OK.
  2. The "X11 forwarding" warning comes from ssh configuration, so it's unrelated to the script.
  3. To me the output from git pull looks good, but the freshly added article is not in the archive, so something did go wrong with git push.

@oliviaguest The error message AttributeError: 'Repository' object has no attribute 'swh' is due to article.py not being up to date. I have just pushed a fix (which I had made this morning for my own use but forgotten to push).

But... you can't just run publish.py again, because the first part of its work (publishing on Zenodo) is already done (and it looks correct). I'd say the best fix now is to create and push the directory for the new article by hand.

What I don't understand is why git flags your publish.py as modified. Did you change something?

@oliviaguest
Copy link
Member

@khinsen:

What I don't understand is why git flags your publish.py as modified. Did you change something?

Yes, due to the "bug" (?) I described in #29 (comment). I added a print statement. I didn't understand how to fix it and ran it with the --zenodo flag without getting the sandbox to work.

@oliviaguest
Copy link
Member

@rougier I don't see a difference [using software tools not my eyes] between my local version and a fresh clone. Sorry. What can I do to fix this? I feel like this is the same bug I had last time (although it was not from my MBP, but from a Linux desktop).

@oliviaguest
Copy link
Member

OK, I did this manually. I think it's OK! ☺️ https://github.com/ReScience/articles/tree/master/10.5281_zenodo.3885793

@sepehrmn
Copy link
Author

sepehrmn commented Jun 9, 2020

Excellent. Many thanks everyone!

@oliviaguest
Copy link
Member

@sepehrmn sorry about the teething problems — I seem to always find a way to screw up right before the end... 😆 BUT! Importantly your paper is published and I think it should appear on the website too soon. So... 🥳 congratulations! 👏

@rougier
Copy link
Member

rougier commented Jun 15, 2020

Did you add the bibtex entry on the website bibfile?

@oliviaguest
Copy link
Member

No, what is needed to be done? Just copy-paste this one? https://github.com/ReScience/articles/blob/master/10.5281_zenodo.3885793/article.bib

@oliviaguest
Copy link
Member

Are the keywords OK? Hmm... See: ReScience/rescience.github.io@2f3b5b6

@rougier
Copy link
Member

rougier commented Jun 15, 2020

Perfect. And yes, you only need to append it to the website bibfile (I'll regenerate the website)

@oliviaguest
Copy link
Member

oliviaguest commented Jun 15, 2020

Great! Excited to see @sepehrmn's paper on the https://rescience.github.io site. 👏

@sepehrmn
Copy link
Author

Excellent! Much appreciated @oliviaguest ! I guess the issue can be closed now!

@oliviaguest
Copy link
Member

Let's make sure everything is OK first.

@sepehrmn
Copy link
Author

Very well. I still can't see it on https://rescience.github.io/read/ .

@oliviaguest
Copy link
Member

@rougier now sure what's up? Have you compiled and it's my manual editing of files that's screwy?

@oliviaguest
Copy link
Member

@sepehrmn feel free to unsubscribe BTW. We will deal with this as it's a journal issue — congrats again and thank you for being so patient! 🥳

@rougier
Copy link
Member

rougier commented Jun 19, 2020

@oliviaguest I'm a bit lost now. What do I need to check exactly ?

@oliviaguest
Copy link
Member

Ah, it's perfect! Sorry for some reason (maybe cache) I couldn't see it! I think everything is fine with this and we can close the issue.
20200619_143653

@schmidDan
Copy link

@oliviaguest is it supposed to be published within the "Issue 2 (NeurIPS 2019 Reproducibility Challenge)" though?

@oliviaguest
Copy link
Member

@schmidDan I am confused! I think I did something wrong again! @rougier? ReScience/ReScience#48 (comment)

@rougier
Copy link
Member

rougier commented Jun 23, 2020

Probably the issue number is wrong. I think it should be 3 and paper number should be #2. You can correct it directly on Zenodo and you'll need to update the published.bib on the rescience website (or I can do it, just tell me).

@oliviaguest
Copy link
Member

oliviaguest commented Jun 23, 2020

@rougier looking on Zenodo, maybe I'm confused... but it looks right? I haven't changed anything.

@rougier
Copy link
Member

rougier commented Jun 29, 2020

Ok, it's only wrong in the bibtex for the website then. I'll correct it.

@oliviaguest
Copy link
Member

Oh! Thank you, @rougier. 🌷

@rougier
Copy link
Member

rougier commented Jun 29, 2020

Done !

@oliviaguest
Copy link
Member

Rejoice! Thanks! 🥳

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

No branches or pull requests

7 participants