Add debugging output to the propagation kernel #902

stephenswat · 2025-03-03T14:37:18Z

This commit adds some optional debugging output to the propagation kernel, allowing it to output more precise statistics on why tracks were killed.

beomki-yeo

I like the PR in general except the naming of failure_mode. Couldn't this be exit_mode or sth like that?

device/common/include/traccc/finding/device/impl/propagate_to_next_surface.ipp

stephenswat · 2025-03-07T09:07:18Z

I've renamed it to exit mode as you requested!

This commit adds some optional debugging output to the propagation kernel, allowing it to output more precise statistics on why tracks were killed.

sonarqubecloud · 2025-03-07T09:08:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

krasznaa

I could imagine a setup in which some shared helper functions would be used by both the CUDA and SYCL code for allocating the debug object in memory, getting it back, and printing it. Since I'm a bit worried about only adding this feature to the CUDA code at the moment. (Not even the host code...?)

krasznaa · 2025-03-07T13:20:12Z

core/include/traccc/finding/details/debug_output.hpp

+void log_propagate_to_next_surface_debug(
+    const propagate_to_next_surface_debug& obj, const Logger& logger);


I wonder what our convention should be for printing custom types. 🤔

I believe if you define

std::ostream& operator<<(std::ostream&, const propagate_to_next_surface_debug&);

, that could be used to print such objects to a traccc::Logger (Acts::Logger) object. Like we do here for instance:

https://github.com/acts-project/traccc/blob/main/performance/include/traccc/performance/timing_info.hpp#L39

https://github.com/acts-project/traccc/blob/main/performance/include/traccc/performance/throughput.hpp#L37

https://github.com/acts-project/traccc/blob/main/examples/run/common/throughput_st.ipp#L214-L225

In general, defining operator<< for a type is an antipattern unless there is a single, ubiquitous way to turn a value of the type into a string. In this case, there are many different wordings and ways to print this data, so operator<< is a no-go. Of course, we can debate that there might be better names for this function.

I think it's pretty safe to define an operator for the "standard way" of printing the object. Which will also allow clients to use the code more flexibly. Printing not just to a logger, but to any kind of stream. And also not just at a DEBUG level, but also any other that they want. (Very realistically we may end up using such a printout in a warning or error message as well. In case the debug information points at some issue.)

So this I feel fairly strongly about.

Consider what happens if we want to print some exit modes at a different severity level than others; then your approach breaks down. I think it's much better to have a function which calls the logger itself, rather than producing a string for the whole object.

krasznaa · 2025-03-07T13:23:57Z

core/include/traccc/finding/details/debug_output.hpp

+enum class propagate_to_next_surface_exit_mode {
+    HOLE_LIMIT_REACHED,
+    BRANCH_LIMIT_REACHED,
+    MAX
+};
+
+struct propagate_to_next_surface_debug {
+    void reset();
+
+    unsigned int m_failure_count[static_cast<std::size_t>(
+        propagate_to_next_surface_exit_mode::MAX)];
+};


This is fairly subjective, but I'd prefer to do this:

namespace traccc::details { struct propagate_to_next_surface_debug_info { enum class exit_mode {...}; unsigned int m_failure_count...; void reset(); }; }

While talking about this... I assume reset() is not meant to be called in device code. But if we explicitly want to prevent users from doing it, it might be helpful to use TRACCC_HOST on it. If not, if technically it may be usable in device code in some circumstances, I would put TRACCC_HOST_DEVICE on it right away. 🤔

krasznaa · 2025-03-07T13:25:06Z

core/include/traccc/finding/finding_config.hpp

@@ -48,6 +48,9 @@ struct finding_config {
    /// Particle hypothesis
    detray::pdg_particle<traccc::scalar> ptc_hypothesis =
        detray::muon<traccc::scalar>();
+
+    /// Provide more verbose output for debugging; may reduce performance
+    bool verbose = false;


I wonder if debug may be a better name here. But I'm really not sure...

krasznaa · 2025-03-07T13:26:45Z

core/src/finding/details/debug_output.cpp

+namespace traccc {
+void propagate_to_next_surface_debug::reset() {
+    for (std::size_t i = 0;
+         i < static_cast<std::size_t>(propagate_to_next_surface_exit_mode::MAX);


Wouldn't std::size(m_failure_count) be more robust here?

Mwah, given that the size of m_failure_count is taken directly from exit_mode::MAX I'd say these are equivalent in terms of robustness.

It's mainly to reduce the number of characters needed to express the same. I agree that it's not more robust code. But it's certainly shorter.

Okay sure, I can use std::size. 👍

krasznaa · 2025-03-07T13:27:39Z

core/src/finding/details/debug_output.cpp

+    if (unsigned int n = obj.m_failure_count[static_cast<std::size_t>(
+            propagate_to_next_surface_exit_mode::HOLE_LIMIT_REACHED)];
+        n > 0) {
+        TRACCC_DEBUG(n << " tracks were killed due to reaching the hole limit");


Using an output operator would also allow us to not hardcode the output level here... 🤔

krasznaa · 2025-03-07T13:28:25Z

device/common/include/traccc/finding/device/propagate_to_next_surface.hpp

+    /**
+     * @brief Optional debug object
+     */
+    propagate_to_next_surface_debug* debug;


Suggested change

propagate_to_next_surface_debug* debug;

propagate_to_next_surface_debug* debug = nullptr;

Then you wouldn't need to touch the SYCL code if you don't want to.

Good idea. 👍

krasznaa · 2025-03-07T13:31:25Z

examples/options/src/track_finding.cpp

@@ -102,6 +107,8 @@ std::unique_ptr<configuration_printable> track_finding::as_printable() const {
        std::to_string(m_config.max_num_skipping_per_cand)));
    cat->add_child(std::make_unique<configuration_kv_pair>(
        "PDG number", std::to_string(m_pdg_number)));
+    cat->add_child(std::make_unique<configuration_kv_pair>(
+        "Debug output", std::format("{}", m_config.verbose)));


If you call it "debug" here as well, then let's make the variable name debug...

stephenswat added feature New feature or request shared Changes related to shared code labels Mar 3, 2025

stephenswat requested review from niermann999 and beomki-yeo March 3, 2025 14:37

stephenswat force-pushed the feat/propagate_debug branch from 8dd1c48 to 0490746 Compare March 3, 2025 14:40

beomki-yeo reviewed Mar 4, 2025

View reviewed changes

device/common/include/traccc/finding/device/impl/propagate_to_next_surface.ipp Outdated Show resolved Hide resolved

stephenswat force-pushed the feat/propagate_debug branch from 0490746 to e6d7f70 Compare March 7, 2025 09:07

stephenswat requested a review from beomki-yeo March 7, 2025 09:07

Add debugging output to the propagation kernel

96a87d3

This commit adds some optional debugging output to the propagation kernel, allowing it to output more precise statistics on why tracks were killed.

stephenswat force-pushed the feat/propagate_debug branch from e6d7f70 to 96a87d3 Compare March 7, 2025 09:07

krasznaa requested changes Mar 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debugging output to the propagation kernel #902

Add debugging output to the propagation kernel #902

stephenswat commented Mar 3, 2025

beomki-yeo left a comment

stephenswat commented Mar 7, 2025

sonarqubecloud bot commented Mar 7, 2025

krasznaa left a comment

krasznaa Mar 7, 2025

stephenswat Mar 10, 2025

krasznaa Mar 10, 2025

stephenswat Mar 10, 2025

krasznaa Mar 7, 2025

krasznaa Mar 7, 2025

krasznaa Mar 7, 2025

stephenswat Mar 10, 2025

krasznaa Mar 10, 2025

stephenswat Mar 10, 2025

krasznaa Mar 7, 2025

krasznaa Mar 7, 2025

stephenswat Mar 10, 2025

krasznaa Mar 7, 2025

		void log_propagate_to_next_surface_debug(
		const propagate_to_next_surface_debug& obj, const Logger& logger);

	propagate_to_next_surface_debug* debug;
	propagate_to_next_surface_debug* debug = nullptr;

Add debugging output to the propagation kernel #902

Are you sure you want to change the base?

Add debugging output to the propagation kernel #902

Conversation

stephenswat commented Mar 3, 2025

beomki-yeo left a comment

Choose a reason for hiding this comment

stephenswat commented Mar 7, 2025

sonarqubecloud bot commented Mar 7, 2025

Quality Gate passed

krasznaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment