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

[VTU] Fixed pvd output #2036

Merged
merged 3 commits into from Jan 10, 2018

Conversation

Projects
None yet
5 participants
@wenqing
Copy link
Member

wenqing commented Jan 9, 2018

This PR fixes #2020.

@chleh

chleh approved these changes Jan 9, 2018

Copy link
Collaborator

chleh left a comment

Only small things.

void Output::doOutputAlways(Process const& process,
const int process_id,
ProcessOutput const& process_output,
unsigned timestep,
const double t,
GlobalVector const& x)
{
const bool make_output =
!(process_id < static_cast<int>(_single_process_data.size()) - 1 &&
!(process.isMonolithicSchemeUsed()));

This comment has been minimized.

@chleh

chleh Jan 9, 2018

Collaborator

I think the expression would be more readable if it was rephrased as:

process_id == static_cast<int>(_single_process_data.size()) - 1 
         || process.isMonolithicSchemeUsed();

Btw.: Is there a special reason why the last process writes output (i.e. process_id == static_cast<int>(_single_process_data.size()) - 1), and not the first one?

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Changed. A comment was added as the reason as

    // For the staggered scheme for the coupling, only the last process, which
    // gives the latest solution within a coupling loop, is allowed to make
    // output.

This comment has been minimized.

@chleh

chleh Jan 10, 2018

Collaborator

OK, thanks.

SingleProcessData* spd_ptr = nullptr;
for (auto spd_it=spd_range.first; spd_it!=spd_range.second; ++spd_it)
{
if(counter == process_id)

This comment has been minimized.

@chleh

chleh Jan 9, 2018

Collaborator

Just curious: Is there a one-to-one correspondence between a process_id and a process in _single_process_data?

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Yes, they are. However In the present code, each process outputs VTU file with all variables of all processes. We may need an option that allows one process only output its own variables in future. Therefore, I keep Output::SingleProcessData for this purpose. Otherwise,

  1. struct Output::SingleProcessData can be removed by enabling MeshLib::IO::PVDFile pvd_file as a member of class Output directly.
  2. std::multimap<Process const*, SingleProcessData> _single_process_data; can be dropped too.

This comment has been minimized.

@chleh

chleh Jan 10, 2018

Collaborator

However In the present code, each process outputs VTU file with all variables of all processes.

But since only the last process writes output, everything is written only once, right?

We may need an option that allows one process only output its own variables in future.

Not sure, because then the grid would be written even more often than it's written now.

This comment has been minimized.

@wenqing

wenqing Jan 10, 2018

Author Member

However In the present code, each process outputs VTU file with all variables of all processes.

But since only the last process writes output, everything is written only once, right?

Yes, it is.

We may need an option that allows one process only output its own variables in future.

Not sure, because then the grid would be written even more often than it's written now.

If it is not needed, I can remove struct Output::SingleProcessData in another PR.

This comment has been minimized.

@chleh

chleh Jan 10, 2018

Collaborator

Yes, a later PR is fine. Maybe, once everything settled, a cleanup of the output logic would be appropriate anyway.

This comment has been minimized.

@wenqing

wenqing Jan 10, 2018

Author Member

Good. Another PR to clean up the output logic including to remove struct Output::SingleProcessData.

@@ -249,10 +235,32 @@ void Output::doOutputNonlinearIteration(Process const& process,
return;
}

const bool make_output =
!(process_id < static_cast<int>(_single_process_data.size()) - 1 &&
!(process.isMonolithicSchemeUsed()));

This comment has been minimized.

@chleh

chleh Jan 9, 2018

Collaborator

Could also be simplified.

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Done.

BaseLib::RunTime time_output;
time_output.start();

findSingleProcessData(process, process_id);
auto spd_range = _single_process_data.equal_range(&process);

This comment has been minimized.

@chleh

chleh Jan 9, 2018

Collaborator

Maybe this entire green block could be moved to a separate method SPD* getSingleProcessData(int process_id) and also used above.

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Added a new member function for that. Since the new member can not have const modifier, the const modifer of doOutputNonlinearIteration was dropped.

!(process_id < static_cast<int>(_single_process_data.size()) - 1 &&
!(process.isMonolithicSchemeUsed()));
doProcessOutput(output_file_path, make_out, _output_file_compression,
doProcessOutput(output_file_path, make_output, _output_file_compression,

This comment has been minimized.

@chleh

chleh Jan 9, 2018

Collaborator

Is it really/still necessary that doProcessOutput() is passed the argument make_output?

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Removed.

This comment has been minimized.

@wenqing

wenqing Jan 9, 2018

Author Member

Added it again. Because doProcessOutput adds variable to vtu although the output can be skipped.

This comment has been minimized.

@chleh

chleh Jan 10, 2018

Collaborator

I see: Writing the output file is skipped if it's not the last process.
Anyway, thanks for your effort.

@wenqing wenqing force-pushed the wenqing:fix_output branch from 05646a1 to 0c4875d Jan 9, 2018

@wenqing

This comment has been minimized.

Copy link
Member Author

wenqing commented Jan 9, 2018

@endJunction It is OK if all benchmarks pass the test.

@bilke

This comment has been minimized.

Copy link
Member

bilke commented Jan 9, 2018

@wenqing Jenkins is fine again.

auto spd_range = _single_process_data.equal_range(&process);
unsigned counter = 0;
int counter = 0;
SingleProcessData* spd_ptr = nullptr;
for (auto spd_it=spd_range.first; spd_it!=spd_range.second; ++spd_it)

This comment has been minimized.

@endJunction

endJunction Jan 9, 2018

Member

There is safe_advance solution on SO https://stackoverflow.com/a/1057782 .
Could be implemented in BaseLib or be done later.

This comment has been minimized.

@wenqing

wenqing Jan 10, 2018

Author Member

This loop will be remove after removing Output::SingleProcessData, which will be presented in another PR.

@endJunction
Copy link
Member

endJunction left a comment

safe advance algorithm can be implemented optionally; can be merged

@wenqing

This comment has been minimized.

Copy link
Member Author

wenqing commented Jan 10, 2018

@bilke Thanks.

@wenqing wenqing merged commit 8df3818 into ufz:master Jan 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@Thomas-TK

This comment has been minimized.

Copy link
Member

Thomas-TK commented Jan 10, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.