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

split simulate into prepare, run and finalize #650

Merged
merged 26 commits into from Mar 17, 2017

Conversation

@apeyser
Contributor

apeyser commented Jan 31, 2017

Needs careful check on simulation_manager, given that it's a conflict bottleneck on master.

As requested by several projects including visualization, this PR makes it possible to split a run into multiple time chunks, without doing the calibration and teardown steps for every run.
Thus, simulate(n) = prepare(); run(n); cleanup();
as well as prepare(); run(n/2); run(n/2); cleanup();

A follow-up issue would be to make a run interruptible, returning the time actually run so that the simulation can be continued with another run call.

split simulate into prepare, run and finalize
Split into run: passes installcheck

pull time out of prepare

prepare_simulation_ code into prepare

prepare_simulation_.2 code into prepare

prepare_simulation_.3 code into prepare

prepare_simulation_.4 code into prepare

prepare_simulation_.all code into prepare

Map Run to Function

Run breakup doesnt break Simulate tests

Add python bindings

Seems to run a tester

Add contextmanager IterateRuns

Split run passes test

test-split fix copyright

Format with clang

Lost manager
@heplesser

@apeyser This is just a first review, I have not evaluated the changes to simulate in detail. Could you explain the rationale for this PR? That would make it easier to understand and evaluate. I also wonder what would happen if one called Run() without having called Prepare() before? What if I forget Cleanup() after Run() and then call Simulate()?

Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated pynest/nest/lib/hl_api_simulation.py
Show outdated Hide outdated pynest/nest/tests/test_split.py
Show outdated Hide outdated pynest/nest/lib/hl_api_simulation.py
Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Feb 1, 2017

Contributor

Do we have a proper image to run clang in? The formatting is tightly fixed to clang-3.6 which is no longer available in many distributions. This is probably a job for microservices! [ @DimitriPlotnikov ]

Contributor

apeyser commented Feb 1, 2017

Do we have a proper image to run clang in? The formatting is tightly fixed to clang-3.6 which is no longer available in many distributions. This is probably a job for microservices! [ @DimitriPlotnikov ]

Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
@@ -443,20 +507,39 @@ nest::SimulationManager::simulate( Time const& t )
"is called repeatedly with simulation times that are not multiples of "

This comment has been minimized.

@wschenck

wschenck Feb 4, 2017

Contributor

Line 502 in new code above: Correct output to "In SimulationManager::run(): "

@wschenck

wschenck Feb 4, 2017

Contributor

Line 502 in new code above: Correct output to "In SimulationManager::run(): "

This comment has been minimized.

@apeyser

apeyser Feb 7, 2017

Contributor

@apeyser

apeyser Feb 7, 2017

Contributor

This comment has been minimized.

@apeyser

apeyser Feb 9, 2017

Contributor

Done.

@apeyser

apeyser Feb 9, 2017

Contributor

Done.

Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
@@ -813,27 +842,6 @@ nest::SimulationManager::update_()
}

This comment has been minimized.

@wschenck

wschenck Feb 4, 2017

Contributor

Additional comment reg. line 791 in the OLD code (in function update_()): I wonder if this part of the code outside of the update loop will work properly if running directly again without a cleanup/prepare cycle. It seems to be related to structural plasticity and updates synaptic elements.
@sdiazpier : May I ask you for help regarding this question?

@wschenck

wschenck Feb 4, 2017

Contributor

Additional comment reg. line 791 in the OLD code (in function update_()): I wonder if this part of the code outside of the update loop will work properly if running directly again without a cleanup/prepare cycle. It seems to be related to structural plasticity and updates synaptic elements.
@sdiazpier : May I ask you for help regarding this question?

This comment has been minimized.

@apeyser

apeyser Feb 7, 2017

Contributor

It's still in the update loop, it's inside of run/start_updating, in the same spot as simulate/resume

@apeyser

apeyser Feb 7, 2017

Contributor

It's still in the update loop, it's inside of run/start_updating, in the same spot as simulate/resume

This comment has been minimized.

@wschenck

wschenck Feb 8, 2017

Contributor

I don't think so. It is outside of the do-while-loop within update_(). Thus it is called only once at the end of run() in the changed code. (In addition, the update of synaptic elements also happens within the do-while-loop, but there is this extra call at the end outside of the loop, and this concerns me slightly - at least it is something to consider.)

@wschenck

wschenck Feb 8, 2017

Contributor

I don't think so. It is outside of the do-while-loop within update_(). Thus it is called only once at the end of run() in the changed code. (In addition, the update of synaptic elements also happens within the do-while-loop, but there is this extra call at the end outside of the loop, and this concerns me slightly - at least it is something to consider.)

This comment has been minimized.

@apeyser

apeyser Feb 9, 2017

Contributor

@wschenck I think I may be confused about the code we're discussing. Could you add the snippet or a link to it? Currently 791 on the master is to:

    } while ( to_do_ > 0 and not exit_on_user_signal_
      and not exceptions_raised.at( thrd ) );

>>>>>    // End of the slice, we update the number of synaptic elements
    for ( std::vector< Node* >::const_iterator i =
            kernel().node_manager.get_nodes_on_thread( thrd ).begin();
          i != kernel().node_manager.get_nodes_on_thread( thrd ).end();
          ++i )
    {
      ( *i )->update_synaptic_elements(
        Time( Time::step( clock_.get_steps() + to_step_ ) ).get_ms() );
    }

  } // end of #pragma parallel omp

which is in the same place in the current monster update_ () function, right after the update loop. That needs to be refactored! 200 lines seperating the do {} while... and the function still has another 100 lines...

@apeyser

apeyser Feb 9, 2017

Contributor

@wschenck I think I may be confused about the code we're discussing. Could you add the snippet or a link to it? Currently 791 on the master is to:

    } while ( to_do_ > 0 and not exit_on_user_signal_
      and not exceptions_raised.at( thrd ) );

>>>>>    // End of the slice, we update the number of synaptic elements
    for ( std::vector< Node* >::const_iterator i =
            kernel().node_manager.get_nodes_on_thread( thrd ).begin();
          i != kernel().node_manager.get_nodes_on_thread( thrd ).end();
          ++i )
    {
      ( *i )->update_synaptic_elements(
        Time( Time::step( clock_.get_steps() + to_step_ ) ).get_ms() );
    }

  } // end of #pragma parallel omp

which is in the same place in the current monster update_ () function, right after the update loop. That needs to be refactored! 200 lines seperating the do {} while... and the function still has another 100 lines...

This comment has been minimized.

@apeyser

apeyser Feb 9, 2017

Contributor

To confirm, do you mean: 669dc49?diff=split#diff-06fecbcc67899362e4c211274b21fb71L791 ?
If so, that code is repeated, once after the loop, and once inside the loop: 669dc49?diff=split#diff-06fecbcc67899362e4c211274b21fb71L791

@apeyser

apeyser Feb 9, 2017

Contributor

To confirm, do you mean: 669dc49?diff=split#diff-06fecbcc67899362e4c211274b21fb71L791 ?
If so, that code is repeated, once after the loop, and once inside the loop: 669dc49?diff=split#diff-06fecbcc67899362e4c211274b21fb71L791

This comment has been minimized.

@wschenck

wschenck Feb 9, 2017

Contributor

Now I am slightly confused about the links in your previous posting because they just open the complete diff.
But you already got the right code snippet in your posting before, concerning the location outside of the do-while-loop. And in line 600-607 (in master) the same code exists within the do-while-loop.
Concerning the outside position of this code snippet, this would be called in a kind of irregular way in subsequent calls to run() (e.g., one run could be for just a fraction of a min-delay interval, and the next for 100 min-delay intervals, and so on...). I just wonder if this may hurt the structural plasticity algorithm. Most likely not, but given the complexity of interacting algorithms in the kernel I am on the paranoid side.

@wschenck

wschenck Feb 9, 2017

Contributor

Now I am slightly confused about the links in your previous posting because they just open the complete diff.
But you already got the right code snippet in your posting before, concerning the location outside of the do-while-loop. And in line 600-607 (in master) the same code exists within the do-while-loop.
Concerning the outside position of this code snippet, this would be called in a kind of irregular way in subsequent calls to run() (e.g., one run could be for just a fraction of a min-delay interval, and the next for 100 min-delay intervals, and so on...). I just wonder if this may hurt the structural plasticity algorithm. Most likely not, but given the complexity of interacting algorithms in the kernel I am on the paranoid side.

This comment has been minimized.

@apeyser

apeyser Feb 9, 2017

Contributor

I see now -- but this would also be a problem for multiple simulate() calls. But this may break the ideas that simulate(n) = prepare(); run(n/2); run(n/2); cleanup()
I think @sdiazpier is the only person who can answer this, or give a good suggestion on what we should do.

@apeyser

apeyser Feb 9, 2017

Contributor

I see now -- but this would also be a problem for multiple simulate() calls. But this may break the ideas that simulate(n) = prepare(); run(n/2); run(n/2); cleanup()
I think @sdiazpier is the only person who can answer this, or give a good suggestion on what we should do.

This comment has been minimized.

@wschenck

wschenck Feb 9, 2017

Contributor

As soon as this is sorted out, I would say 👍 for pulling!

@wschenck

wschenck Feb 9, 2017

Contributor

As soon as this is sorted out, I would say 👍 for pulling!

This comment has been minimized.

@sdiazpier

sdiazpier Feb 9, 2017

Contributor

Dear @apeyser and @wschenck, sorry for the late reply. The update of the synaptic elements can be done in any irregular fraction of simulation time. This is not relevant because the structural changes actually take place on a much larger time scale, so the accumulation of these small changes in the synaptic elements can be achieved at partial steps and the final results should be the same.
To verify this I ran the simple structural plasticity examples and they provide the same output using the new Prepare() - Run() - Cleanup() functions as with Simulate()

@sdiazpier

sdiazpier Feb 9, 2017

Contributor

Dear @apeyser and @wschenck, sorry for the late reply. The update of the synaptic elements can be done in any irregular fraction of simulation time. This is not relevant because the structural changes actually take place on a much larger time scale, so the accumulation of these small changes in the synaptic elements can be achieved at partial steps and the final results should be the same.
To verify this I ran the simple structural plasticity examples and they provide the same output using the new Prepare() - Run() - Cleanup() functions as with Simulate()

This comment has been minimized.

@apeyser

apeyser Feb 10, 2017

Contributor

@apeyser

apeyser Feb 10, 2017

Contributor

@sdiazpier

Great job @apeyser, this was very much needed for many interactive applications and will benefit a lot the structural plasticity usage and monitoring.

@Silmathoron

This looks cool, thanks @apeyser!

Show outdated Hide outdated nestkernel/nest.h
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Feb 10, 2017

Contributor

@heplesser Any more suggestions before merging?

Contributor

apeyser commented Feb 10, 2017

@heplesser Any more suggestions before merging?

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Feb 10, 2017

Contributor

Issue #659 has been added as a follow up to this PR, to add state checks for the prepare-run-cleanup loop.

Contributor

apeyser commented Feb 10, 2017

Issue #659 has been added as a follow up to this PR, to add state checks for the prepare-run-cleanup loop.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Feb 12, 2017

Contributor

@apeyser Thanks for your efforts. I will try to take a look within the next few days.

Contributor

heplesser commented Feb 12, 2017

@apeyser Thanks for your efforts. I will try to take a look within the next few days.

@jougs

@apeyser: thanks for this nice work. I could not see any problems in the code other than the two minor things I've pointed out in my comments. As I think the test is good enough, I did not run the new code myself.

!! PLEASE DON'T MERGE UNTIL AFTER THE RELEASE OF 2.12.0 !!

Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated pynest/nest/tests/test_split.py
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Feb 27, 2017

Contributor

@jougs I'll rebase on 2.12.0 (hopefully that's happening very soon). Then the whole thing can be merge into nestio.

Contributor

apeyser commented Feb 27, 2017

@jougs I'll rebase on 2.12.0 (hopefully that's happening very soon). Then the whole thing can be merge into nestio.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Feb 27, 2017

Contributor

We're going to release current master as 2.12.0 probably already tomorrow.

Contributor

jougs commented Feb 27, 2017

We're going to release current master as 2.12.0 probably already tomorrow.

@heplesser

@apeyser Thank you for all the improvements you have made. I have some more suggestions, mainly on documentation and names. I would also feel safer if we had a bit of control between parts, especially a check that quits run() if prepare() has not been called since program start or last cleanup().

Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated pynest/nest/lib/hl_api_simulation.py
Show outdated Hide outdated pynest/nest/lib/hl_api_simulation.py
Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
Show outdated Hide outdated nestkernel/simulation_manager.cpp
@heplesser

@apeyser Almost there now, I would just like some more minor fixes to documentation, plus one comment on hl_api_simulation.py:55 that you didn't fix yet.

Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated nestkernel/nestmodule.cpp
Show outdated Hide outdated pynest/nest/lib/hl_api_simulation.py
@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 8, 2017

Contributor

@heplesser @jougs I add some of the suggestion from hep that affected the node-manager, and now with the new vera++ analysis, node_manager fails with 65 errors -- apparently the file was very out of sink with vera++, and as soon as the file is changed at all, all fixes must be made.

That doesn't seem to be in the scope of this pull request, but should be a separate issue to correct this file. What should we do about this?

Contributor

apeyser commented Mar 8, 2017

@heplesser @jougs I add some of the suggestion from hep that affected the node-manager, and now with the new vera++ analysis, node_manager fails with 65 errors -- apparently the file was very out of sink with vera++, and as soon as the file is changed at all, all fixes must be made.

That doesn't seem to be in the scope of this pull request, but should be a separate issue to correct this file. What should we do about this?

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 8, 2017

Contributor

@apeyser I just created #672 to fix the Vera++ issues, including a problem with our Vera++ rules. Could you also merge from master (should work without a problem) to get the new static code checking in its final version in place here?

Contributor

heplesser commented Mar 8, 2017

@apeyser I just created #672 to fix the Vera++ issues, including a problem with our Vera++ rules. Could you also merge from master (should work without a problem) to get the new static code checking in its final version in place here?

@apeyser

This comment has been minimized.

Show comment
Hide comment
@apeyser

apeyser Mar 17, 2017

Contributor

Added apeyser/nest-simulator#1. I believe that it's only formatting changes -- please correct if I'm wrong @heplesser

Contributor

apeyser commented Mar 17, 2017

Added apeyser/nest-simulator#1. I believe that it's only formatting changes -- please correct if I'm wrong @heplesser

@apeyser apeyser merged commit 4a373f7 into nest:master Mar 17, 2017

1 check passed

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

@apeyser apeyser deleted the apeyser:resplit branch Mar 17, 2017

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 19, 2017

Contributor

@apeyser Almost all changes were formatting (adding curly braces around one-line blocks, ! -> not, line length, spaces), a few tiny spelling corrections and in three places I changed "undefined behavior (the famed nose-inhabiting monkeys of internet lore)" to "undefined behavior and incorrect results" in comments (nestmodule.cpp, l 490, 520, 543).

Contributor

heplesser commented Mar 19, 2017

@apeyser Almost all changes were formatting (adding curly braces around one-line blocks, ! -> not, line length, spaces), a few tiny spelling corrections and in three places I changed "undefined behavior (the famed nose-inhabiting monkeys of internet lore)" to "undefined behavior and incorrect results" in comments (nestmodule.cpp, l 490, 520, 543).

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