Improvement of iterative solution framework #273

Merged
merged 6 commits into from Apr 22, 2016

Conversation

Projects
None yet
5 participants
@janhahne
Contributor

janhahne commented Mar 10, 2016

This PR improves the iterative solution framework used for connections without delay (so far only gap junctions, but there is more to come).

  • Renamed several internal and kernel variables from rather unspecific names to names of typ wfr_* in order to indicate that they belong to the iterative waveform relaxation (wfr) scheme.
max_num_prelim_iterations -> wfr_max_iterations
prelim_tol -> wfr_tol
prelim_interpolation_order -> wfr_interpolation_order
prelim_update -> wfr_update
needs_prelim_update -> node_uses_wfr
...
  • Added kernel variable wfr_comm_interval. All connections without delay contribute with wfr_comm_interval to the min_delay and max_delay. The communication of the waveform relaxation method uses the regular communication points in intervals of the minimal network delay. Therefore it is desirable to be able to influence the min_delay for simulations with connections without delay
  • Added kernel flag use_wfr to disable usage of iterative waveform relaxation method and use communication in every step instead ( single step method ). This is achieved by setting wfr_comm_interval = resolution and will be useful for rate models without delay where wfr is not always necessary and to reproduce data of the single step method from our gap-junction paper.
  • Added a new unit test test_wfr_settings.sli that checks that wfr_comm_interval and use_wfr work properly and can only be set in a reasonable order. Adapted test_gap_junction.sli due to functionality of wfr_comm_interval.
  • Improved and corrected Python-Code in both gap-junction examples

Most certainly there will be a small conflict with PR #269 in node_manager.cpp. I will adapt this PR once #269 is merged.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 21, 2016

Contributor

I suggest @tammoippen and @jschuecker as reviewers.

Contributor

jougs commented Mar 21, 2016

I suggest @tammoippen and @jschuecker as reviewers.

+ 'weight': j_exc,
+ 'delay': delay})
+
+nest.Connect(neurons,sd,'all_to_all')

This comment has been minimized.

@jschuecker

jschuecker Mar 23, 2016

Contributor

Since all_to_all is default you could leave it out.

@jschuecker

jschuecker Mar 23, 2016

Contributor

Since all_to_all is default you could leave it out.

nestkernel/connector_model_impl.h
+ {
+ kernel().connection_builder_manager.get_delay_checker().assert_valid_delay_ms( delay );
+ }
+ // Let connections without delay contribute to min_delay with wfr_comm_interval

This comment has been minimized.

@jschuecker

jschuecker Mar 23, 2016

Contributor

to min_delay and max_delay, right? However the contribution to max_delay is not important, so I am not sure if we should mention both here!?

@jschuecker

jschuecker Mar 23, 2016

Contributor

to min_delay and max_delay, right? However the contribution to max_delay is not important, so I am not sure if we should mention both here!?

@jschuecker

This comment has been minimized.

Show comment
Hide comment
@jschuecker

jschuecker Mar 23, 2016

Contributor

@janhahne, is there a reason, why the pynest examples are not in pynest/examples?

I made only two minor comments in line. 👍 otherwise.

Contributor

jschuecker commented Mar 23, 2016

@janhahne, is there a reason, why the pynest examples are not in pynest/examples?

I made only two minor comments in line. 👍 otherwise.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Mar 30, 2016

Contributor

@jschuecker Thank you for the review! I addressed your two comments in the new commit.

There is no particular reason why the examples are in examples/nest instead of in pynest/examples. Should they be moved there?

Contributor

janhahne commented Mar 30, 2016

@jschuecker Thank you for the review! I addressed your two comments in the new commit.

There is no particular reason why the examples are in examples/nest instead of in pynest/examples. Should they be moved there?

nestkernel/connector_model_impl.h
+ else
+ {
+ kernel().connection_builder_manager.get_delay_checker().assert_valid_delay_ms(
+ kernel().simulation_manager.get_wfr_comm_interval() );

This comment has been minimized.

@tammoippen

tammoippen Mar 31, 2016

Contributor

If I understand correctly, the wfr_comm_interval is set only during SimulationManager::set_status and cannot change otherwise. If yes, then we need to check for a valid value only once (not for every 'delay-less' connection) when it is changed in the SimulationManager or when when min/max_delay is change in the ConnectionBuilderManager. Also relevant for the subsequent changes in this file.

Edit: I just saw, that we only need to check, when there is actually such a connection created. But still we only need to check it once (as this is similar to a default delay of all 'delay-less' connections). I propose to surround the check in the add_connection with an if ( default_delay_needs_check_ ) and set it afterwards.

@tammoippen

tammoippen Mar 31, 2016

Contributor

If I understand correctly, the wfr_comm_interval is set only during SimulationManager::set_status and cannot change otherwise. If yes, then we need to check for a valid value only once (not for every 'delay-less' connection) when it is changed in the SimulationManager or when when min/max_delay is change in the ConnectionBuilderManager. Also relevant for the subsequent changes in this file.

Edit: I just saw, that we only need to check, when there is actually such a connection created. But still we only need to check it once (as this is similar to a default delay of all 'delay-less' connections). I propose to surround the check in the add_connection with an if ( default_delay_needs_check_ ) and set it afterwards.

+ 'wfr_comm_interval': 1.0,
+ 'wfr_tol': 0.0001,
+ 'wfr_max_iterations': 15,
+ 'wfr_interpolation_order': 3})

This comment has been minimized.

@tammoippen

tammoippen Mar 31, 2016

Contributor

Why do you have two subsequent calls to SetKernelStatus? Can't you call it once like this:

nest.SetKernelStatus({'resolution': 0.05, 
                      'total_num_virtual_procs': threads,
                      'print_time': True,
# comments ...
                      'use_wfr': True,
                      'wfr_comm_interval': 1.0,
                      'wfr_tol': 0.0001,
                      'wfr_max_iterations': 15,
                      'wfr_interpolation_order': 3})

(Similar in the other example.)

@tammoippen

tammoippen Mar 31, 2016

Contributor

Why do you have two subsequent calls to SetKernelStatus? Can't you call it once like this:

nest.SetKernelStatus({'resolution': 0.05, 
                      'total_num_virtual_procs': threads,
                      'print_time': True,
# comments ...
                      'use_wfr': True,
                      'wfr_comm_interval': 1.0,
                      'wfr_tol': 0.0001,
                      'wfr_max_iterations': 15,
                      'wfr_interpolation_order': 3})

(Similar in the other example.)

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 14, 2016

Contributor

@tammoippen Thank you for taking a look and your comments!

I improved the calls to SetKernelStatus in the examples. Regarding your second comment I am not quite sure, if I understand your suggestion correctly. You are right, that wfr_comm_interval is set only during SimulationManager::set_status and you are also right, that it can be interpreted as a default delay for connections without delay (which is however used anyway, even if the user sets a different delay for the connection).

So I see your point, that once one 'delay-less' connection is created it is no longer necessary to call assert_valid_delay_ms() for any further 'delay-less' connection, because the min/max delay is set correctly. I am however not sure how to achieve this in a smart way. Could you give me another hint (or if easier for you create a PR to my branch)?

Contributor

janhahne commented Apr 14, 2016

@tammoippen Thank you for taking a look and your comments!

I improved the calls to SetKernelStatus in the examples. Regarding your second comment I am not quite sure, if I understand your suggestion correctly. You are right, that wfr_comm_interval is set only during SimulationManager::set_status and you are also right, that it can be interpreted as a default delay for connections without delay (which is however used anyway, even if the user sets a different delay for the connection).

So I see your point, that once one 'delay-less' connection is created it is no longer necessary to call assert_valid_delay_ms() for any further 'delay-less' connection, because the min/max delay is set correctly. I am however not sure how to achieve this in a smart way. Could you give me another hint (or if easier for you create a PR to my branch)?

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 22, 2016

Contributor

@tammoippen Please see commit 0505f81. I think I found a solution. I would appreciate a quick merge of this pull request in order to move forward with the rate model implementation.

Contributor

janhahne commented Apr 22, 2016

@tammoippen Please see commit 0505f81. I think I found a solution. I would appreciate a quick merge of this pull request in order to move forward with the rate model implementation.

+ kernel().simulation_manager.get_wfr_comm_interval() );
+ default_delay_needs_check_ = false;
+ }
+

This comment has been minimized.

@tammoippen

tammoippen Apr 22, 2016

Contributor

This solution looks very good. Maybe it should be placed at the start of the function (line 354ff), because if something is bad with the delay, the connection is not stored in the connection infrastructure.

@tammoippen

tammoippen Apr 22, 2016

Contributor

This solution looks very good. Maybe it should be placed at the start of the function (line 354ff), because if something is bad with the delay, the connection is not stored in the connection infrastructure.

This comment has been minimized.

@janhahne

janhahne Apr 22, 2016

Contributor

Good catch, thanks!

@janhahne

janhahne Apr 22, 2016

Contributor

Good catch, thanks!

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Apr 22, 2016

Contributor

When my last note is addressed, I am all for merging.

Contributor

tammoippen commented Apr 22, 2016

When my last note is addressed, I am all for merging.

@janhahne

This comment has been minimized.

Show comment
Hide comment
@janhahne

janhahne Apr 22, 2016

Contributor

Great, thanks! Since @jschuecker also gave his 👍, this should be ready for merging now :)

Contributor

janhahne commented Apr 22, 2016

Great, thanks! Since @jschuecker also gave his 👍, this should be ready for merging now :)

@heplesser heplesser merged commit a2fdc2b into nest:master Apr 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@janhahne janhahne deleted the janhahne:gap_framework_improvement branch Apr 25, 2016

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