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

Move flush from finalize to new function post_run_cleanup() #692

Merged
merged 5 commits into from Mar 24, 2017

Conversation

@jougs
Copy link
Contributor

@jougs jougs commented Mar 23, 2017

This is related to #650 and moves flushing of files from Cleanup (i.e. SimulationManager::finalize()) to the end of Run (i.e. SimulationManager::run()).

Copy link
Contributor

@heplesser heplesser left a comment

Some minor comments, looks good otherwise.

@@ -299,11 +299,21 @@ class Node
virtual void calibrate() = 0;

/**
* Cleanup node after Run.
* Override this function if a node needs to "wrap up" things after
* a call to Run, i.e., before SimulationManager::run(). Typical

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

A bit confused by the before SimulationManager::run()---why not after?

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

Should have been "before SimulationManager::run() returns.". Fixed.

#ifdef _OPENMP
#pragma omp parallel
{
index t = kernel().vp_manager.get_thread_id();

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Should be const

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

  1. Yes, it could be. However, I just copied the code from the finalize() function and did not want to change or refactor the functionality at this point. The code will also go away when nestio is merged.
#endif // clang-format on
for ( size_t idx = 0; idx < local_nodes_.size(); ++idx )
{
Node* node = local_nodes_.get_node_by_index( idx );

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Couldn't you use the begin() and end() iterators of local_nodes_ to avoid lookup by index?

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

See 1. above.

if ( B_.fs_.is_open() )
{
if ( P_.flush_after_simulate_ )
B_.fs_.flush();

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Curly braces missing.

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

  1. Yes. And also in hundreds of other places in this file. Can't I just disable the checker until #691 is merged? I don't really see a reason for fixing this all over again...
if ( P_.flush_after_simulate_ )
B_.fs_.flush();

if ( !B_.fs_.good() )

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Should be not.

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

See 2. above.


if ( !B_.fs_.good() )
{
std::string msg =

This comment has been minimized.

@heplesser

heplesser Mar 23, 2017
Contributor

Could be const

This comment has been minimized.

@jougs

jougs Mar 23, 2017
Author Contributor

See 1. above.

@wschenck
Copy link
Contributor

@wschenck wschenck commented Mar 23, 2017

I have no objections to add. Nice work. I endorse the pull request. 👍

jougs added 2 commits Mar 23, 2017
@heplesser heplesser requested a review from wschenck Mar 24, 2017
@heplesser heplesser merged commit 0eac914 into nest:master Mar 24, 2017
1 check passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.