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

Update models to accept parameter objects as node parameters #2482

Merged
merged 4 commits into from
Oct 19, 2022

Conversation

hakonsbm
Copy link
Contributor

Some models were not updated to take parameter objects when setting parameters such as V_m. With this PR they are updated, and a test of setting V_m for all relevant node models is added. Fixes #2480.

@hakonsbm hakonsbm added T: Bug Wrong statements in the code or documentation S: High Should be handled next I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) labels Sep 27, 2022
@hakonsbm hakonsbm added this to To do in Models via automation Sep 27, 2022
@clinssen
Copy link
Contributor

Thanks Håkon! Just to be sure: nodes do not need to implement set( const DictionaryDatum& ) any longer on their State_ and Parameters_ members, but only set( const DictionaryDatum&, Node* )?

@hakonsbm
Copy link
Contributor Author

@clinssen Yes, to be able to use a parameter object to set model parameters, the node pointer has to be passed when using set() of State_ and Parameters_. This is because the parameter object may be based on spatial information, which it gets from the node.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@hakonsbm Thanks! It looks good, but I wonder if you could extend the test to test setting all parameters that can be set. I believe there is a test somewhere that does something like this, it may be a SLI test. Could you see if you can find that?

I'd also suggest to call the test test_regression_issue-2480.py in line with the other Python regression tests we have.

@clinssen
Copy link
Contributor

@hakonsbm: could you point me to the changes I should make to make this work for synapses as well? I'm getting the same error when trying syn.w = nest.random.uniform(0., 1.) but passing this from within the C++ synapse object to updateValueParam() does not work; it only takes Node objects.

@hakonsbm
Copy link
Contributor Author

@heplesser I have moved the test to a new file. I can't find the test you mention which sets all parameters that can be set, though. I also thought about how one could test all settable parameters, but there are a couple problems with that. Getting all parameters that can be set would be complicated. And some node parameters require their values being lower or higher than other parameters, which is impossible to check without consulting the documentation for that model. Ultimately I think a test like that, while useful, would be too complicated and difficult to make reliable.

@hakonsbm
Copy link
Contributor Author

@clinssen Parameter objects that set parameters of non-spatial connections requires the post-synaptic node. So to set the status of a synapse model, you would have to pass the node you get here:

const Node* target = kernel().node_manager.get_node_or_proxy( target_node_id, tid );

It would then have to be passed as an argument down to the model's set_status() function.

For spatial connections it's a bit more complicated, because you then need not only the post-synaptic node, but also the layer, source node position, and target node position.

However, using parameter objects to set parameters of synapses is already possible when creating the connections.

Copy link
Contributor

@heplesser heplesser left a comment

Choose a reason for hiding this comment

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

@hakonsbm Thanks, looks good to me! Your point that a test checking all parameters would be too difficult, so I think the test we have now is good.

@clinssen
Copy link
Contributor

So this is not possible/expected to fail with the same TypeMismatch exception?
https://github.com/nest/nestml/pull/818/files#diff-fb6a53f8c7efa8bb222864d58cd7a6fd6dd09cc9082324f71cacdff743074150R76

@hakonsbm
Copy link
Contributor Author

@clinssen Yes, without the changes outlined in my previous comment it's impossible to set parameters of a synapse with a parameter object after the connection is created.

Copy link
Contributor

@clinssen clinssen left a comment

Choose a reason for hiding this comment

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

The corresponding NESTML PR now contains a working test for setting both neuron and synapse variables with distributions, so I can now pass off on this. Cheers!

@heplesser heplesser merged commit d4d6aa2 into nest:master Oct 19, 2022
@hakonsbm hakonsbm deleted the model_vm branch October 19, 2022 08:23
@clinssen clinssen moved this from To do to Done in Models Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: High Should be handled next T: Bug Wrong statements in the code or documentation
Projects
Models
  
Done
Development

Successfully merging this pull request may close these issues.

Cannot set membrane voltage to random values for several models
3 participants