Skip to content

ContourTree cleanup

The ContourTree filters, worklets, and associated helpers have some issues that need to be addressed. At a minimum, the coding conventions document should be followed: Coding conventions.

Some of the issues that need to be addressed:

  • Any functional public class should be in its own header file with the same name as the class. The filter and worklet names should match the file names to make them easier to locate.
  • Always spell out words in names; do not use abbreviations. E.g. PPP2.
  • Classes are documented with Doxygen-style comments before classes, methods, and functions. The filters should have documentation on how they're used, what they do, what inputs they expect, what they output, etc.
  • All class, method, member variable, and functions should start with a capital letter. Local variables should start in lower case and then use camel case. Several function and variable names break this convention.
  • Some helper classes are defined in the global namespace. These should be moved into an appropriate vtkm namespace.
  • The algorithm currently leaks memory (e.g. ContourTreeUniformAugmented.hxx:497). Such issues are considered critical and must be addressed before this code can be considered production-ready. Future patches should have such issues resolved before making merge requests.
  • Timing/benchmarking code should be removed or converted to use the scoped logging functions provided by VTK-m, which can track execution times consistently with the rest of the project. Worklet executions are already logged at the Perf level, any additionally timing code should be similar. See vtkm/cont/Logging.h for more info.

There may be other issues as well. The authors of this feature should review the full set of coding conventions for VTK-m and make a pass through their code to check for conformance. Running the tests through valgrind/memcheck or similar to check for additional memory leaks would be a good idea, too.

@oruebel @ghweber @dcamp I'm not sure who to assign this to, so I'll let you guys decide who wants to take this on.