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

Also change out_stream condition when enable/disable_output #16726

Merged
merged 1 commit into from Apr 17, 2024

Conversation

RichardYCJ
Copy link
Contributor

This will close #16725

Comment on lines 1018 to +1021
TimerOutput::disable_output()
{
output_is_enabled = false;
out_stream.set_condition(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is right. What TimerOutput::enable/disable_output() does is determine whether the TimerOutput object wants to output something. Whether the underlying out_stream object actually wants to show the resulting text is a separate question, unrelated to what the current function does. In particular, if you call TimerOutput::disable_output(), then with your patch you would also suppress output from all other places that might write into the ConditionalOStream object. That is not right.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, first of all, the out_stream is owned by the TimerOutput itself and so enable/disable it would not effect the outer ConditionalOStream. It only copies the input condition and construct its own, or set to active if input a ostream. And to your first concern, if the timer itself doesn't want to output anything, it won't harm if you set its own out_stream to inactive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand the architecture of the class now. What's not yet clear to me is why setting output_is_enabled=false is not enough. What is the place that is creating output even if that flag is set to false? That seems like the underlying problem. It's true that we can fix this by calling out_stream.set_condition(false); as you do here, but we already document the current function as follows:

  /**
   * By calling this function, all output can be disabled. This function
   * together with enable_output() can be useful if one wants to control the
   * output in a flexible way without putting a lot of <tt>if</tt> clauses in
   * the program.
   */
  void
  disable_output();

In other words, if you call the function, it sets output_is_enabled to false, and that should disable all output to out_stream. It should not be necessary to let out_stream suppress anything that's sent to them; it should just not be receiving anything to begin with.

Or what is it that I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look into the source file of timer.cc, output_is_enabled is only checked when ~TimerOutput() is invoked, and thus disable/enable_output only works when you construct it and let it print when destruct it.

But in most time, I need to output it multiple times by manually calling print_wall_time_statistics() or print_summary(). They do not check this flag and it is hard to add so (also contains MPI func, cannot simply add a high-level check).

So the better way is just deactivating the out_stream since we don't need the output.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I agree that the design of this class is pretty broken in this regard. Thanks for debugging it and for patiently explaining it to me!

@RichardYCJ RichardYCJ requested a review from bangerth April 9, 2024 02:57
@bangerth
Copy link
Member

/rebuild

@kronbichler kronbichler merged commit b3da75b into dealii:master Apr 17, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ConditionalOStream status change after initialization
3 participants