nutofem / nuto

NuTo - yet another finite element library
https://nuto.readthedocs.io
Boost Software License 1.0
17 stars 5 forks source link

Pde dynamic poly visualization #188

Closed TTitscher closed 6 years ago

TTitscher commented 6 years ago

TL/DR:

// Before:
Visualizer<AverageHandler> visu(cells, AverageGeometryQuad());

// After:
Visualizer visu(cells, AverageGeometryQuad());

Detailed:

Changes the Visualizer to take a HandlerInterface instead of a template THandler. For the case of AverageHandler and VoronoiHandler those handlers are created internally if you provide a AverageGeometry or a VoronoiGeometry.

I think the separation of the Visualizer and the Handler is great, but IMO we do not have to pay the compile time cost. Plus, the interface is a tiny bit shorter, as shown above. With the prototype pattern (HandlerInterface::Clone()) you are still free to define custom handlers in you main files.

A new feature is Visualizer::SetCells() that allows you to redefine the cells. This is handy if you implement your problem as a class and the Visualizer is a member of such class. In this case, the cells have to be available in the constructor of your class, which might cause problems. Now, you can initialize the Visualizer with an empty cell group (mVisualizer({}, AverageGeometryLine())) and change them later.

PS: The diff in the unit tests is somehow messed up. It is basically a moving and renaming.

TTitscher commented 6 years ago

The common handler interface ok. But whats with your rule of three? Maybe it is too early too clean up.

I did not change the actual handler interface (ok, added Clone()), so I think is not really a cleanup. Anyways, as a fan of the rule of three, I added a third build-in handler, the PointHandler. So I guess the interface is alright.

And I somehow think that the Visualizer should not know what Handlers are available.

We can certainly discuss that. I can easily remove this, which would result in a call like

// 1) Before:
Visualizer<AverageHandler> visu(cells, AverageGeometryQuad());

// 2) After (Visualizer knows AverageHandler)
Visualizer visu(cells, AverageGeometryQuad());

// 3) After (Visualizer knows nothing)
Visualizer visu(cells, AverageHandler(AverageGeometryQuad()));

I thought 2) for convenience, we can do 3) as well. However, I tend to use 2) since this should cover almost all our cases and is worth the specialization.

Psirus commented 6 years ago
  1. I agree that the visualizer should not have a list of possible handlers. a) this almost amounts to making the handler and the geometry one again, and b) "why should I pay the compile time costs" ;)
  2. SetCells is a good idea.
  3. static vs dynamic handlers: I don't have a strong opinion on whether we should do the handler statically or dynamically; I originally choose static because the implementation was easier (to me). However, if we do decide to go the dynamic route, I'd like to know how much of a difference the "OMG, so much compile" really is. Have you benchmarked it? What is the run-time cost of a dynamic solution? Does the gain justify rewriting it? (Not that that is important anymore, since you already have).

But in the end, I'm willing to accept the PR, because the work is done and the compile time will go down (even though we don't know by how much or whether this is the only way to achieve this, but anyway...)

TTitscher commented 6 years ago

Alright, removing the handler in Visualizer: 2 vs. 1 :stuck_out_tongue_closed_eyes: I will fix that.

Another advantage of the non-template Visualizer is its use other classes. At some point, we should write a wrapper that collects all instances of Visualizers of the problem together with their visualize functions. So that we end up with something like OldNuTo::PostProcess() with PostProcess::Visualize(time) that automatically writes vtu and pvd. Lugging this template parameter around all those classes may suck.