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

Reproducibility #57

Merged
merged 6 commits into from
Jan 29, 2024
Merged

Reproducibility #57

merged 6 commits into from
Jan 29, 2024

Conversation

mlober
Copy link

@mlober mlober commented Jan 17, 2024

This PR aims to improve reproducibility by setting the default parameters to those used to produce the dynamics of the metastable state in Schmidt et al. (2018). It also removes redundancies such as the stabilization matrix K_stable.npy and unused parameters in default_parameters.py.

In the README and code one now finds a small description on how to run the model in the ground and metastable state. This might need refining.

In addition, it adds a missing object "structure_revered" to the model class that is needed for analysis.

@albada
Copy link
Collaborator

albada commented Jan 19, 2024

All of it looks good to me. I just think the new initial mean membrane potential and the newly introduced structure_reversed require a little bit of documentation describing the reasoning behind these.

@didi-hou
Copy link
Collaborator

I could have found the problem that K_stable was set as None instead of the numpy file K_stable.npy in default parameters (also for the full_scale MAM). As in our earlier meeting with Sacha, Markus, Hans, and Renan, the interareal probabilistic connectivity was found to generate Non-existing connectivity in the connectivity matrix of down-scaled MAM and Renan and I were wondering why. My recent code backtrace found that in down-scaled MAM, the K_stable is set as K_stable.npy, but in the default_params.py, K_stable = None. Thus, thanks to @mlober, this PR should also fix this problem.

@didi-hou didi-hou requested review from albada and shimoura January 20, 2024 15:03
@didi-hou didi-hou added the bug Something isn't working label Jan 20, 2024
@albada
Copy link
Collaborator

albada commented Jan 21, 2024

Why did K_stable = None produce non-existing connectivity? Shouldn't this simply use the connectivity matrix as determined from the anatomical data?

@didi-hou
Copy link
Collaborator

Why did K_stable = None produce non-existing connectivity? Shouldn't this simply use the connectivity matrix as determined from the anatomical data?

What I meant is that: in the current version of MAM, K_stable is set as "K_stable.npy" in down-scaled MAM, but as None in full-scale MAM (default parameters). The connectivity matrix created by down-scaled MAM contains connections that do not appear in the connectivity matrix of full-scale MAM. HEP points out this problem in our earlier meeting. Now I suspect that it's caused by the K_stable being set as None in full-scale MAM, but "K_stable.npy" in down-scaled MAM. But I still need to check and confirm after this PR is merged. Does this make sense?

@albada
Copy link
Collaborator

albada commented Jan 22, 2024

OK, thanks for clarifying.

Copy link
Collaborator

@albada albada left a comment

Choose a reason for hiding this comment

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

All of it looks good to me. I just think the new initial mean membrane potential and the newly introduced structure_reversed require a little bit of documentation describing the reasoning behind these.

@albada
Copy link
Collaborator

albada commented Jan 22, 2024

I would add a comment to the code along the lines of "The initial membrane potential distribution is chosen to be centered on a hyperpolarized state to limit synchronous initial firing, which could bring the model into a high-activity state when the parameters are set according to the metastable condition."

Copy link
Member

@shimoura shimoura left a comment

Choose a reason for hiding this comment

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

Thanks for finding these issues and improving the code. In addition to @albada suggestions, I only pointed out one more thing regarding the parameter cc_weights_I_factor, please check and confirm if what I wrote is correct.

multiarea_model/default_params.py Show resolved Hide resolved
run_example_downscaled.py Outdated Show resolved Hide resolved
@didi-hou didi-hou requested review from albada and shimoura January 29, 2024 08:36
@albada albada merged commit 008afdc into INM-6:master Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants