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] An anatomically constrained neural network model of fear conditioning #60

Open
davinellulinvega opened this issue Oct 11, 2021 · 27 comments

Comments

@davinellulinvega
Copy link

Original article:
Armony, J. L., Servan-Schreiber, D., Cohen, J. D., & LeDoux, J. E. (1995). An anatomically constrained neural network model of fear conditioning. Behavioral Neuroscience, 109(2), 246–257. https://doi.org/10.1037/0735-7044.109.2.246

PDF URL:
https://github.com/davinellulinvega/ReArmony/blob/master/article.pdf
Metadata URL:
https://github.com/davinellulinvega/ReArmony/blob/master/metadata.yaml
Code URL:
https://gitlab.com/davinellulinvega/armony

Scientific domain:
Computational Neuroscience
Programming language:
Python
Suggested editor:
@eroesch

@rougier
Copy link
Member

rougier commented Oct 18, 2021

Thanks for your submission. An editor will be soon assigned.

@eroesch @oliviaguest @benoit-girard Can one of you edit this submission ?

@eroesch
Copy link

eroesch commented Oct 18, 2021

Yes @rougier, I can do.

@eroesch
Copy link

eroesch commented Oct 18, 2021

In fact, I have done some work using the original paper myself at some point :)

@davinellulinvega
Copy link
Author

Well thank you very much in advance for you time and consideration.

@rougier
Copy link
Member

rougier commented Nov 5, 2021

@eroesch gentle reminder

@benoit-girard
Copy link

@eroesch : I may serve as a reviewer, let me know if you want me to review this one.

@eroesch
Copy link

eroesch commented Nov 22, 2021

Thanks @benoit-girard (@rougier). Actually, I hadn't realised when I accepted the editing of that one, that I have actually interacted with the authors a while back, as a examiner for a phd, on a slightly different version of the code. I had then suggested they submit to ReSci C. I am happy to carry on as editor (and I am almost done scrutinising the code, which is straightforward), but perhaps it would be sensible to have a second pair of eyes, if you have time to spare? @rougier, do you have an opinion?

@rougier
Copy link
Member

rougier commented Nov 22, 2021

For me, I think it is fine. Especially since our review are open anyway. If you favor the author, everyone will know :)

@rougier
Copy link
Member

rougier commented Dec 20, 2023

Just realised this is two years old and no message since then. Very sorry about such huge delay.
@davinellulinvega Are you still interested in having you work reviewed?

@davinellulinvega
Copy link
Author

@rougier I am indeed still interested in having this article published, if possible.

@rougier
Copy link
Member

rougier commented Jan 9, 2024

@davinellulinvega We'll do your best and again deepest apologies. @eroesch can you go on with editing? @benoit-girard proposed himself as reviewer and I can be the second one.

And I'm afraid we alreaky broke the record of longest submission time...

@benoit-girard
Copy link

I can deliver a first review of this work.

PDF

The paper, it is clear and well organized.

Concerning the twofold increase of the amygdala activity reported in the original paper, it is hard to judge if it is indeed the case in the replication, based on fig. 7 only. Could the authors replicate fig. 4A of the original paper?

In the paper, the authors refer to themselves with the first person ("I", "me") while there are two of them. Wouldn't it be more coherent to either write "we" or indicate which author is writing what in the paper? It would be unethical to publish a paper with two authors if only one of them performed the study.

Code

The install command for requirements "> pip install -U -r requirements.txt" returns an error : "install: illegal option -- U" could this line be corrected?

@eroesch
Copy link

eroesch commented Jan 18, 2024

Hi! Yes, I can take over the editing.

@davinellulinvega
Copy link
Author

@benoit-girard Thank you very much for your feedback. I have updated the article's content to make Figure 7 clearer, and removed any mention of "I".
Regarding the repository, I have updated the Readme as well to remove the -U when setting up the requirements with pip.

@benoit-girard
Copy link

@davinellulinvega thanks, I now have an error suggesting that the install of pytables is not working:

`girard$ python3 main.py
INFO:root:Executing the development phase ...
INFO:root:Done
INFO:root:Frequency chosen to be the CS: 14
INFO:root:Executing the conditioning phase ...
INFO:root:Done
INFO:root:Recording the results ...
Traceback (most recent call last):
File "/usr/local/lib/python3.11/site-packages/pandas/compat/_optional.py", line 142, in import_optional_dependency
module = importlib.import_module(name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/Cellar/[email protected]/3.11.3/Frameworks/Python.framework/Versions/3.11/lib/python3.11/importlib/init.py", line 126, in import_module
return _bootstrap._gcd_import(name[level:], package, level)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "", line 1206, in _gcd_import
File "", line 1178, in _find_and_load
File "", line 1142, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'tables'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/Users/girard/Evaluations/ReScience-Armony-2024/armony-master/main.py", line 206, in
extract_and_save()
File "/Users/girard/Evaluations/ReScience-Armony-2024/armony-master/main.py", line 167, in extract_and_save
final.to_hdf(DATA_PATH, key=k, complevel=9, mode='a',
File "/usr/local/lib/python3.11/site-packages/pandas/core/generic.py", line 2682, in to_hdf
pytables.to_hdf(
File "/usr/local/lib/python3.11/site-packages/pandas/io/pytables.py", line 302, in to_hdf
with HDFStore(
^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pandas/io/pytables.py", line 560, in init
tables = import_optional_dependency("tables")
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/usr/local/lib/python3.11/site-packages/pandas/compat/_optional.py", line 145, in import_optional_dependency
raise ImportError(msg)
ImportError: Missing optional dependency 'pytables'. Use pip or conda to install pytables.`

However I installed HDF5 and the installation of the requirements with pip install -r requirements.txt resulted in the installation of:

Successfully installed blosc2-2.4.0 msgpack-1.0.7 ndindex-1.7 numexpr-2.8.8 py-cpuinfo-9.0.0 tables-3.9.2

Maybe pytables was not installed? Is it correctly checked for in the requirements?

@davinellulinvega
Copy link
Author

@benoit-girard That is a very odd error. Pytables is explicitly stated in the requirements.txt (under the simple name of tables). Plus, it seems that your execution of pip install -r requirements.txt has successfully setup tables.
What kind of environment are you using to test the script? I just made a new pyenv virtual environment on linux to try and reproduce the error, but everything seems to be running fine.

@benoit-girard
Copy link

I was trying to have the code running with a macOS system. I switched to Linux and it worked smoothly. The review of the code should arrive soon.

@benoit-girard
Copy link

benoit-girard commented Feb 5, 2024

The code is very clear and commented, I don't think any modification is necessary.

I could run the code with a Ubuntu 22.04.1 system, following the installation steps. I could then generate figures identical to those in the paper.

This closes my review.

@davinellulinvega
Copy link
Author

@benoit-girard Thank you very much for your review.
To try and avoid any issues on different OSs, I have added the hdf5 dependency on the pandas package in the requirements file.

@rougier
Copy link
Member

rougier commented Feb 12, 2024

Installation

I'm on OSX and beside the virtualenv step (that I skipped), everything went smoothly. I had to adapt the apt-get with brew and only had to install hdf5 instead of hdf5-tools.

Running

No problem and I check all the figures are identical to the ones in the article.

Code

Code is exceptionally clear (and commented). Well done. You need to add a License to your code though (else it is cannot be read). Also, foir publication, you'll need to have your repository saved on Software Heritage

Article

Article is well written and very easy to follow. Authors explain both the context, the goal and the results. They underlined missing information from the original and the values they have been using (which is one of the goal of the replication) and confirm results on the original paper.

Some questions

  • Figure 1 is your own, right (else we need ot ask permission from original pulisher). You could indicate size of structures (with n = xxx) but not mandatory at all. Figure is nice as it is.
  • You explain that size of layers were not given but aren't they given by figure 1 in the original article ?

Conclusion

@eroesch Article can be published provided a license is added to the code.
@davinellulinvega It's a shame it took us more than two years to make the review. Again, very sorry for that.

@eroesch
Copy link

eroesch commented Feb 12, 2024

@davinellulinvega Really sorry for the delay, it's really my fault!

@eroesch
Copy link

eroesch commented Feb 12, 2024

@davinellulinvega I'll take you through the editorial process. I will clone your repo, make some tweaks and submit a PR.

@davinellulinvega
Copy link
Author

@rougier Thank you very much for your feedback. I can confirm that figure 1 in the submitted paper is my own. Regarding the number of neural units per layer, you are correct that figure 1 in the original paper does contain the right amount per layer. However, nowhere are those numbers confirmed in the original text, hence my statement within the article. If you find it unfair, I can modify the sentence to better reflect reality.
In terms of license, is there any constraints that I should be aware of? Or would a CC-BY be enough?

@eroesch Thank you very much in advance for your help. I will keep an eye on my mail box for any notification.

@rougier
Copy link
Member

rougier commented Feb 19, 2024

@davinellulinvega You could add a simple sentence saying just that (the number of units might indicate in figure 1 but without confirmation anywhere in the text). For the code license, you can use https://choosealicense.com/. For Rescience C, we ask for open source license such that anybody can reuse your code. Typically, GPL v3 or BSD are fine.

@davinellulinvega
Copy link
Author

@rougier Thank you very much for your answer. As per your advice, I have added a GPL v3 license to the source code, and modified the article's content to let the reader know of the situation regarding the number of neural units in the various layers.
In the next few days, I will also submit a request to Software Heritage to have the repository saved.

@eroesch
Copy link

eroesch commented Feb 23, 2024

@davinellulinvega
Copy link
Author

@eroesch You are entirely correct. A few days ago, I investigated the archival process. I expected to have to provide some proof or confirmation of my ownership of the git repository, before the project would be approved for recording. Instead, the request was processed in just a few moments.
However, since I have now added the License, I will request a new snapshot be created.

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

4 participants