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

add Event signals for GMG #5913

Closed
wants to merge 1 commit into from
Closed

add Event signals for GMG #5913

wants to merge 1 commit into from

Conversation

tjhei
Copy link
Member

@tjhei tjhei commented Feb 16, 2018

Before going further with this, I am hoping to get some feedback (@kronbichler, @davydden, others?) on an implementation of specific events inside the library for the user to hook into (for logging, timing, debugging, etc.). See #2298 for the original question.

The current implementation here does what I need (see the test for an example). I assume we want this to be a more generic thing, so it makes sense to move the declaration of the events into a header in base/ ?

Any other comments?

@bangerth
Copy link
Member

Why not use separate signal variables like we do in Triangulation that one can subscribe to? The solution with the enums seems clunky.

}

/**
* Implementation of the multigrid method. The implementation supports both
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably did not need to indent this comment so that git does not show it as a diff.

@davydden
Copy link
Contributor

davydden commented Feb 16, 2018

Why not use separate signal variables like we do in Triangulation that one can subscribe to?

I guess the reason is that you don't know in advance how many levels you have in GMG and you may want to track operations on each level separately (i.e. get timing for different levels).

@tjhei
Copy link
Member Author

tjhei commented Feb 17, 2018

Why not use separate signal variables like we do in Triangulation that one can subscribe to? The solution with the enums seems clunky.

Interesting point. I guess one of the reasons to do it this way is that one of the use cases I have in mind is subscribing to everything and logging all events including time stamps (see the included test as an example). That would be annoying to do if I had to write a lambda for every single event type.

@masterleinad
Copy link
Member

I like the idea. Currently, we are using a lot of Timer objects in all the objects passed to Multigrid. It would be much cleaner to just use signals for that.

@tjhei tjhei changed the title [WIP] add Event signals for GMG add Event signals for GMG Feb 20, 2018
@bangerth
Copy link
Member

I find the use of an enum not much more elegant than the idea of having separate signals for separate things. What if you just passed a string instead? If you want to log everything, then presumably you're not interested in being able to enumerate up front all of the possibilities (or you'd have invented different signals to begin with), and you can live with the fact that the enumeration happens implicitly in the form of strings that are different but whose values you won't know in advance?

@tjhei
Copy link
Member Author

tjhei commented Feb 21, 2018

What if you just passed a string instead?

I am not totally opposed to it, but I think it is a bit more fragile. One of Conrad's use cases is something like this:

auto lambda = [&](const Event& e)
{
      if (e.name==Event::coarse_solve)
	{
	      if (e.type==Event::start)
		timer_coarse.start();
	      else
		timer_coarse.stop();
	}
      if (e.name==Event::smoother_step && e.level==max_level)
	// ...
};

Of course I can do everything with string comparisons, but I like having as much of the information in a structured way. I don't really want to parse log strings/files...

@bangerth
Copy link
Member

Then make the event take 3 arguments: one string to identify what it is, a bool for start/stop, and a level.

int K;
};


Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep it simple and not have varying coefficients

@guidokanschat
Copy link
Contributor

Dear all, @masterleinad and @jwitte08 just discussed the issue. We prefer a solution which is closer to Triangulation, namely different signals for different purposes. Reasons for this are mainly

  1. Exensibility: no enum lists are needed, if another signal is required, another handler can be added without affecting the existing ones. This is even true if the new handler needs a different set of arguments.
  2. Simplicity, less 'logic' in handler functions. Since the handler does not have to differentiate, it can often be a one-liner.
  3. Compatibility: following the example of Triangulation, we only put the most important information into function arguments, the rest can be obtained from the object.

We add a code sample, which illustrates using timers and outputting a vector

Multigrid<dim> mg;
std::vector<Timer> timers(n_levels);

auto timer_handler = [](int level, bool start)
{
  deallog << "Event: a " << e.level << std::endl;
  timers[level].set_status(start);
};

auto vector_handler = [&](int level, bool start)
{
  deallog << "Event: b " << e.level << std::endl;
  print_vector(mg.solution[level]);
};

mg.connect_transfer(timer_handler);
mg.connect_smoother(vector_handler);

// solve

// output accumulated timers

Note: Daniel prefers a reference to the multigrid object in the function calls, thus

auto timer_handler = [](const Mulrigrid<dim>&, int level, bool start)
{
  deallog << "Event: a " << e.level << std::endl;
  timers[level].set_status(start);
};

@jppelteret
Copy link
Member

Why not use separate signal variables like we do in Triangulation that one can subscribe to?

different signals for different purposes

I don't know too much about MG, but to me this seems like a perfectly reasonable and flexible solution to the issue at hand. I see there's a mixed opinion on the matter in the preceding discussion, but @guidokanschat's clearly demonstrated that with such an interface its possible to treat events on a per-level basis. Retaining implementation consistency by mirroring the existing signals mechanism for the Triangulation class is, IMO, favourable.

@bangerth
Copy link
Member

Note: Daniel prefers a reference to the multigrid object in the function calls, thus

auto timer_handler = [](const Mulrigrid<dim>&mg, int level, bool start)
{
  ...do something with mg...
};

If you define the lambda at the same location where you also attach it to the multigrid object, then you can achieve the same effect by passing this as a capture argument:

Multigrid mg;     // some variable with a sufficiently long lifetime
...
auto timer_handler = [mg](int level, bool start)    // capture 'mg' by reference
{
  ...do something with mg...
};
...
mg.signals.timer.connect (timer_handler);

This way, the mg object doesn't have to pass a reference to itself to the signal.

But I think the additional argument also has merits for cases where one wants to call larger functions not as well suited to lambdas from a signal. Not sure we do this anywhere, but I can think of cases -- in fact, on second thought, SolutionTransfer does these sorts of things. In that case, it does make sense to pass the triangulation/dofhandler/multigrid objects as arguments.

So one can argue either way. I'd argue for being consistent, taking into account what we already do in such places.

@masterleinad
Copy link
Member

So one can argue either way. I'd argue for being consistent, taking into account what we already do in such places.

It seems that we don't pass the object as argument for the SolverGMRES, SolverCG, ParameterAcceptor and Triangulation signals.

@guidokanschat
Copy link
Contributor

Superceded by #6129

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.

None yet

6 participants