Clean up logging upon calls to Simulate #269

Merged
merged 9 commits into from Mar 24, 2016

Conversation

Projects
None yet
4 participants
@jougs
Contributor

jougs commented Mar 9, 2016

The output upon calls to Simulate has previously been spread out over multiple messages and cluttered by useless messages. This was hard to read by humans and hard to parse by machines. This PR cleans and compacts logging messages and adds the number of threads and processes to the output in order to prevent problems like seen in #238.

The old output looked like this:

Mar 09 18:22:58 Simulate [Info]: 
    Simulating 100 ms.

Mar 09 18:22:58 NodeManager::prepare_nodes_ [Info]: 
    Please wait. Preparing elements.

Mar 09 18:22:58 NodeManager::prepare_nodes_ [Info]: 
    Simulating 1000 local nodes.

Mar 09 18:22:58 SimulationManager::update [Info]: 
    Simulating using OpenMP.

Mar 09 18:22:58 SimulationManager::resume [Info]: 
    Simulation finished.
OpenMP
Mar 09 18:22:58 NodeManager::finalize_nodes() [Info]: 
     using OpenMP.

The new output for the same script looks like this:

Mar 09 18:26:55 NodeManager::prepare_nodes [Info]: 
    Preparing 1000 nodes for simulation.

Mar 09 18:26:55 SimulationManager::resume [Info]: 
    Number of local nodes: 1000
    Simulaton time (ms): 100
    Number of OpenMP threads: 2
    Number of MPI processes: 1

Mar 09 18:26:55 SimulationManager::resume [Info]: 
    Simulation finished.

If NEST was compiled without support for OpenMP, the third line of the table will read

Not using OpenMP

If NEST was compiled without support for MPI, the fourth line of the table will read

Not using MPI

While testing with 0 nodes, I found a segfault, which I then also fixed in this PR for convenience reasons.

jougs added some commits Mar 9, 2016

Fixed segfault when simulating 0 nodes.
Setting nodes_vec_network_size_ to 0 forces the nodes_vec to be resized to the current number of threads. If this is omitted, calibration reaches behind the nodes_vec and causes a segfault.
Cleaned up logging for calls to Simulate
* Removed unuseful messages
* Added number of threads and number of processes to output
* Reformatted logging message into a table
@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 9, 2016

Contributor

@apeyser, @heplesser, would you please review? Thanks!

Contributor

jougs commented Mar 9, 2016

@apeyser, @heplesser, would you please review? Thanks!

nestkernel/simulation_manager.cpp
nest::SimulationManager::prepare_simulation_()
{
- if ( to_do_ == 0 )
- return;
-

This comment has been minimized.

@jougs

jougs Mar 9, 2016

Contributor

This could never be reached, as the calling function already has a check for t != 0

@jougs

jougs Mar 9, 2016

Contributor

This could never be reached, as the calling function already has a check for t != 0

This comment has been minimized.

@apeyser

apeyser Mar 10, 2016

Contributor

Then it should have an assert and a comment on the invariant

@apeyser

apeyser Mar 10, 2016

Contributor

Then it should have an assert and a comment on the invariant

nestkernel/simulation_manager.cpp
@@ -298,7 +298,7 @@ nest::SimulationManager::simulate( Time const& t )
to_do_ += t.get_steps();
to_do_total_ = to_do_;
- prepare_simulation_();
+ size_t num_active_nodes = prepare_simulation_();

This comment has been minimized.

@apeyser

apeyser Mar 10, 2016

Contributor

This is a const, correct?

@apeyser

apeyser Mar 10, 2016

Contributor

This is a const, correct?

finalize_simulation_();
}
void
-nest::SimulationManager::resume_()
+nest::SimulationManager::resume_( size_t num_active_nodes )

This comment has been minimized.

@apeyser

apeyser Mar 10, 2016

Contributor

Is this (by the way const) num_active_nodes commented as simply a logging value and not actually usable to alter the number of active nodes? Or should this be a SimulationManager member? Or a node manager member?

@apeyser

apeyser Mar 10, 2016

Contributor

Is this (by the way const) num_active_nodes commented as simply a logging value and not actually usable to alter the number of active nodes? Or should this be a SimulationManager member? Or a node manager member?

nestkernel/node_manager.cpp
- for ( index t = 0; t < kernel().vp_manager.get_num_threads(); ++t )
- {
+ for ( index t = 0; t < kernel().vp_manager.get_num_threads(); ++t )
+ {

This comment has been minimized.

@tammoippen

tammoippen Mar 10, 2016

Contributor

You managed to trick clang-format: it thinks these two lines should be indented one more level. This is why TravisCI complains. Either you indent them, or surround them with // clang-format off and // clang-format on or we need to think off something else (clang-format-3.8 still has the same problem).

@tammoippen

tammoippen Mar 10, 2016

Contributor

You managed to trick clang-format: it thinks these two lines should be indented one more level. This is why TravisCI complains. Either you indent them, or surround them with // clang-format off and // clang-format on or we need to think off something else (clang-format-3.8 still has the same problem).

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 10, 2016

Contributor

I've added an assert and constness as suggested by @apeyser and disabled clang-format for some lines as suggested by @tammoippen. Many thanks for the thorough review.

Contributor

jougs commented Mar 10, 2016

I've added an assert and constness as suggested by @apeyser and disabled clang-format for some lines as suggested by @tammoippen. Many thanks for the thorough review.

nestkernel/nest.cpp
- Time t = Time::ms( time );
-
- kernel().simulation_manager.simulate( t );
+ kernel().simulation_manager.simulate( Time::ms( time ) );

This comment has been minimized.

@heplesser

heplesser Mar 10, 2016

Contributor

Merging may lead to a conflict with #265, but that should be easy to handle.

@heplesser

heplesser Mar 10, 2016

Contributor

Merging may lead to a conflict with #265, but that should be easy to handle.

nestkernel/node_manager.h
*/
- void prepare_nodes();
+ const size_t prepare_nodes();

This comment has been minimized.

@heplesser

heplesser Mar 10, 2016

Contributor

I think the const here makes little sense and would remove it.

@heplesser

heplesser Mar 10, 2016

Contributor

I think the const here makes little sense and would remove it.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 10, 2016

Contributor

@jougs Great and 👍 once you have moved the const, see my and @apeyser's comments in the code.

Contributor

heplesser commented Mar 10, 2016

@jougs Great and 👍 once you have moved the const, see my and @apeyser's comments in the code.

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Contributor

@jougs The conflict is due to my merging #265, where I added parameter checking to simulate() in nest.cpp. I think that should be rather easy to sort out.

Contributor

heplesser commented Mar 11, 2016

@jougs The conflict is due to my merging #265, where I added parameter checking to simulate() in nest.cpp. I think that should be rather easy to sort out.

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 11, 2016

Contributor

@heplesser: I've merged in your changes and removed the const-ness for prepare_nodes()

Contributor

jougs commented Mar 11, 2016

@heplesser: I've merged in your changes and removed the const-ness for prepare_nodes()

@heplesser

This comment has been minimized.

Show comment
Hide comment
@heplesser

heplesser Mar 11, 2016

Contributor

@jougs 👍 from me once CI is green, but I think @apeyser would appreciate a const on line 301 in simulation_manager.cpp, see his comment above.

Contributor

heplesser commented Mar 11, 2016

@jougs 👍 from me once CI is green, but I think @apeyser would appreciate a const on line 301 in simulation_manager.cpp, see his comment above.

-#endif
+#else // clang-format off
+ for ( index t = 0; t < kernel().vp_manager.get_num_threads(); ++t )
+ {

This comment has been minimized.

@tammoippen

tammoippen Mar 21, 2016

Contributor

This for-loop start should be indented one more level.

@tammoippen

tammoippen Mar 21, 2016

Contributor

This for-loop start should be indented one more level.

This comment has been minimized.

@jougs

jougs Mar 21, 2016

Contributor

No, it shouldn't. Depending on the definition of _OPENMP, the compiler either sees

#pragma omp parallel
  {

or

  for (...)
  {

The for statement in line in (new) line 727 is not inside the #pragma omp parallel block, but replaces it conditionally. So the indentation level of the opening curly brace in line 724 and (new) line 728 have to match.

As clang-format does apparently not take conditional compilation into account, I had to disable it to get the correct indentation.

@jougs

jougs Mar 21, 2016

Contributor

No, it shouldn't. Depending on the definition of _OPENMP, the compiler either sees

#pragma omp parallel
  {

or

  for (...)
  {

The for statement in line in (new) line 727 is not inside the #pragma omp parallel block, but replaces it conditionally. So the indentation level of the opening curly brace in line 724 and (new) line 728 have to match.

As clang-format does apparently not take conditional compilation into account, I had to disable it to get the correct indentation.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Mar 21, 2016

Contributor

Maybe we should also include the MPI rank in the status message or limit the status message to rank 0?

Contributor

tammoippen commented Mar 21, 2016

Maybe we should also include the MPI rank in the status message or limit the status message to rank 0?

@jougs

This comment has been minimized.

Show comment
Hide comment
@jougs

jougs Mar 21, 2016

Contributor

Only printing on rank 0 seems strange to me, as the number of local nodes might differ and this message might be very useful for debugging.

But I agree that rank information would be useful. However, I think it would be better if that was added globally to the LOG function to have it included in all output. I can open a corresponding PR if you think that's a good idea.

Contributor

jougs commented Mar 21, 2016

Only printing on rank 0 seems strange to me, as the number of local nodes might differ and this message might be very useful for debugging.

But I agree that rank information would be useful. However, I think it would be better if that was added globally to the LOG function to have it included in all output. I can open a corresponding PR if you think that's a good idea.

@tammoippen

This comment has been minimized.

Show comment
Hide comment
@tammoippen

tammoippen Mar 24, 2016

Contributor

Missed the indentation. 👍 for this PR. (merging)

I think, both rank and thread would be useful information for LOG - in another PR.

Contributor

tammoippen commented Mar 24, 2016

Missed the indentation. 👍 for this PR. (merging)

I think, both rank and thread would be useful information for LOG - in another PR.

@tammoippen tammoippen merged commit 78d7d07 into nest:master Mar 24, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment