improved type consistency

networmix · networmix · commit 150fd12725e3 · 2025-09-09T21:34:02.000-07:00
diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp
@@ -1,11 +1,9 @@
 /*
   Pybind11 module exposing NetGraph-Core C++ APIs to Python.
-
-  Notes:
-    - Accepts NumPy arrays (C-contiguous) and converts to spans for zero-copy
-      views where possible.
-    - Distances returned as float64 arrays with inf for unreachable.
-    - Edge/Node masks are validated for dtype=bool and length.
+  - Uses NumPy arrays with zero-copy spans where possible
+  - Returns distances as float64 arrays (inf for unreachable)
+  - Validates edge/node masks for bool dtype and length
+  - Array-returning view methods expose non-owning buffers over internal state; treat as read-only
 */
 #include <pybind11/pybind11.h>
 #include <pybind11/numpy.h>
@@ -82,13 +80,12 @@ PYBIND11_MODULE(_netgraph_core, m) {
             auto dst_s = as_span<std::int32_t>(dst, "dst");
             if (src_s.size() != dst_s.size()) throw py::type_error("src and dst must have the same length");
             auto cap_s = as_span<double>(capacity, "capacity");
-            // Require cost dtype to be int64 for consistency
+            // Cost dtype must be int64 to match internal Cost type
             auto cost_s = as_span<std::int64_t>(cost, "cost");
-            std::span<const Cost> cost_cs(reinterpret_cast<const Cost*>(cost_s.data()), cost_s.size());
             return StrictMultiDiGraph::from_arrays(num_nodes,
                                                   src_s,
                                                   dst_s,
-                                                  cap_s, cost_cs, add_reverse);
+                                                  cap_s, cost_s, add_reverse);
           },
           py::arg("num_nodes"), py::arg("src"), py::arg("dst"), py::arg("capacity"), py::arg("cost"),
           py::kw_only(), py::arg("add_reverse") = false)
@@ -97,7 +94,7 @@ PYBIND11_MODULE(_netgraph_core, m) {
       // external link ids removed; EdgeId is the canonical id
       .def("capacity_view", [](py::object self_obj, const StrictMultiDiGraph& g){
         auto s = g.capacity_view();
-        return py::array(
+        py::array out(
             py::buffer_info(
                 const_cast<double*>(s.data()),
                 sizeof(double),
@@ -108,10 +105,11 @@ PYBIND11_MODULE(_netgraph_core, m) {
             ),
             self_obj
         );
+        return out;
       })
       .def("edge_src_view", [](py::object self_obj, const StrictMultiDiGraph& g){
         auto s = g.edge_src_view();
-        return py::array(
+        py::array out(
             py::buffer_info(
                 const_cast<std::int32_t*>(s.data()),
                 sizeof(std::int32_t),
@@ -122,10 +120,11 @@ PYBIND11_MODULE(_netgraph_core, m) {
             ),
             self_obj
         );
+        return out;
       })
       .def("edge_dst_view", [](py::object self_obj, const StrictMultiDiGraph& g){
         auto s = g.edge_dst_view();
-        return py::array(
+        py::array out(
             py::buffer_info(
                 const_cast<std::int32_t*>(s.data()),
                 sizeof(std::int32_t),
@@ -136,10 +135,11 @@ PYBIND11_MODULE(_netgraph_core, m) {
             ),
             self_obj
         );
+        return out;
       })
       .def("cost_view", [](py::object self_obj, const StrictMultiDiGraph& g){
         auto s = g.cost_view();
-        return py::array(
+        py::array out(
             py::buffer_info(
                 const_cast<Cost*>(s.data()),
                 sizeof(Cost),
@@ -150,6 +150,7 @@ PYBIND11_MODULE(_netgraph_core, m) {
             ),
             self_obj
         );
+        return out;
       })
       .def("row_offsets_view", [](const StrictMultiDiGraph& g){
         auto s = g.row_offsets_view();
@@ -446,24 +447,18 @@ PYBIND11_MODULE(_netgraph_core, m) {
       })
       .def("capacity_view", [](py::object self_obj, const FlowState& fs){
         auto s = fs.capacity_view();
-        return py::array(
-            py::buffer_info(
-                const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }
-            ), self_obj);
+        py::array out(py::buffer_info(const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }), self_obj);
+        return out;
       })
       .def("residual_view", [](py::object self_obj, const FlowState& fs){
         auto s = fs.residual_view();
-        return py::array(
-            py::buffer_info(
-                const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }
-            ), self_obj);
+        py::array out(py::buffer_info(const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }), self_obj);
+        return out;
       })
       .def("edge_flow_view", [](py::object self_obj, const FlowState& fs){
         auto s = fs.edge_flow_view();
-        return py::array(
-            py::buffer_info(
-                const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }
-            ), self_obj);
+        py::array out(py::buffer_info(const_cast<double*>(s.data()), sizeof(double), py::format_descriptor<double>::format(), 1, { s.size() }, { sizeof(double) }), self_obj);
+        return out;
       })
       .def("place_on_dag", [](FlowState& fs, std::int32_t src, std::int32_t dst, const PredDAG& dag, double requested_flow, FlowPlacement placement, bool shortest_path){
         py::gil_scoped_release rel; auto placed = fs.place_on_dag(src, dst, dag, requested_flow, placement, shortest_path); py::gil_scoped_acquire acq; return placed;
@@ -630,8 +625,8 @@ PYBIND11_MODULE(_netgraph_core, m) {
            py::arg("diminishing_returns_epsilon_frac") = 1e-3)
       .def("flow_count", &FlowPolicy::flow_count)
       .def("placed_demand", &FlowPolicy::placed_demand)
-      .def("place_demand", [](FlowPolicy& p, FlowGraph& fg, std::int32_t src, std::int32_t dst, std::int32_t flowClass, double volume, py::object target_per_flow, py::object min_flow){ std::optional<double> tpf; if (!target_per_flow.is_none()) tpf = py::cast<double>(target_per_flow); std::optional<double> mfl; if (!min_flow.is_none()) mfl = py::cast<double>(min_flow); py::gil_scoped_release rel; auto pr = p.place_demand(fg, src, dst, flowClass, volume, tpf, mfl); py::gil_scoped_acquire acq; return py::make_tuple(pr.first, pr.second); }, py::arg("flow_graph"), py::arg("src"), py::arg("dst"), py::arg("flowClass"), py::arg("volume"), py::arg("target_per_flow") = py::none(), py::arg("min_flow") = py::none())
-      .def("rebalance_demand", [](FlowPolicy& p, FlowGraph& fg, std::int32_t src, std::int32_t dst, std::int32_t flowClass, double target){ py::gil_scoped_release rel; auto pr = p.rebalance_demand(fg, src, dst, flowClass, target); py::gil_scoped_acquire acq; return py::make_tuple(pr.first, pr.second); },
+      .def("place_demand", [](FlowPolicy& p, FlowGraph& fg, std::int32_t src, std::int32_t dst, FlowClass flowClass, double volume, py::object target_per_flow, py::object min_flow){ std::optional<double> tpf; if (!target_per_flow.is_none()) tpf = py::cast<double>(target_per_flow); std::optional<double> mfl; if (!min_flow.is_none()) mfl = py::cast<double>(min_flow); py::gil_scoped_release rel; auto pr = p.place_demand(fg, src, dst, flowClass, volume, tpf, mfl); py::gil_scoped_acquire acq; return py::make_tuple(pr.first, pr.second); }, py::arg("flow_graph"), py::arg("src"), py::arg("dst"), py::arg("flowClass"), py::arg("volume"), py::arg("target_per_flow") = py::none(), py::arg("min_flow") = py::none())
+      .def("rebalance_demand", [](FlowPolicy& p, FlowGraph& fg, std::int32_t src, std::int32_t dst, FlowClass flowClass, double target){ py::gil_scoped_release rel; auto pr = p.rebalance_demand(fg, src, dst, flowClass, target); py::gil_scoped_acquire acq; return py::make_tuple(pr.first, pr.second); },
            py::arg("flow_graph"), py::arg("src"), py::arg("dst"), py::arg("flowClass"), py::arg("target"))
       .def("remove_demand", [](FlowPolicy& p, FlowGraph& fg){ py::gil_scoped_release rel; p.remove_demand(fg); py::gil_scoped_acquire acq; })
       .def_property_readonly("flows", [](const FlowPolicy& p){ py::dict out; for (auto const& kv : p.flows()) { const auto& idx = kv.first; const auto& f = kv.second; out[py::make_tuple(idx.src, idx.dst, idx.flowClass, idx.flowId)] = py::make_tuple(f.src, f.dst, f.cost, f.placed_flow); } return out; });
diff --git a/include/netgraph/core/flow_graph.hpp b/include/netgraph/core/flow_graph.hpp
@@ -1,4 +1,4 @@
-/* Shared flow ledger layering over FlowState with per-flow deltas. */
+/* FlowGraph manages per-flow edge deltas over FlowState. */
 #pragma once
 
 #include <cstdint>
@@ -14,9 +14,8 @@
 
 namespace netgraph::core {
 
-// FlowGraph is a shared, authoritative flow ledger over a StrictMultiDiGraph.
-// It composes a FlowState for residual and aggregate edge_flow management, and
-// maintains per-flow edge deltas to support exact removal/reopt.
+// FlowGraph manages per-flow edge deltas over a StrictMultiDiGraph.
+// Composes FlowState for residual/aggregate edge flow management.
 class FlowGraph {
 public:
   explicit FlowGraph(const StrictMultiDiGraph& g);
@@ -30,8 +29,7 @@ class FlowGraph {
   // Access underlying graph (const)
   [[nodiscard]] const StrictMultiDiGraph& graph() const noexcept { return *g_; }
 
-  // Placement: applies placement and records per-edge deltas for this flow.
-  // Returns placed amount.
+// Apply placement and record per-edge deltas for this flow. Returns placed amount.
   [[nodiscard]] Flow place(const FlowIndex& idx, NodeId src, NodeId dst,
              const PredDAG& dag, Flow amount,
              FlowPlacement placement, bool shortest_path = false);
@@ -40,23 +38,22 @@ class FlowGraph {
   void remove(const FlowIndex& idx);
 
   // Remove all flows belonging to a given flowClass.
-  void remove_by_class(std::int32_t flowClass);
+  void remove_by_class(FlowClass flowClass);
 
   // Reset all state to initial capacity and clear ledger.
   void reset() noexcept;
 
   // Inspect: return a copy of the flow's edges and amounts.
   [[nodiscard]] std::vector<std::pair<EdgeId, Flow>> get_flow_edges(const FlowIndex& idx) const;
 
-  // Attempt to reconstruct a single path (LSP) for this flow from the ledger.
-  // Returns empty vector if the flow does not correspond to a unique simple path
-  // (e.g., when placed with multipath/proportional splitting).
+// Reconstruct single path for this flow from ledger.
+// Returns empty vector if flow uses multipath/proportional splitting.
   [[nodiscard]] std::vector<EdgeId> get_flow_path(const FlowIndex& idx) const;
 
 private:
   const StrictMultiDiGraph* g_ {nullptr};
   FlowState fs_;
-  // Per-flow ledger: only edges with non-zero assigned flow are stored.
+  // Per-flow ledger: stores only edges with non-zero flow
   std::unordered_map<FlowIndex, std::vector<std::pair<EdgeId, Flow>>, FlowIndexHash> ledger_;
 };
 
diff --git a/include/netgraph/core/flow_policy.hpp b/include/netgraph/core/flow_policy.hpp
@@ -1,7 +1,4 @@
-/*
-  FlowPolicy — policy engine managing flows for a single demand.
-  See src/flow_policy.cpp for detailed behavior notes.
-*/
+/* FlowPolicy manages flows for a single demand. */
 #pragma once
 
 #include <cstdint>
@@ -22,7 +19,7 @@ namespace netgraph::core {
 
 enum class PathAlg : std::int32_t { SPF = 1 };
 
-// Execution context bundles algorithms and graph handle for clear dependency injection
+// Execution context with algorithms and graph handle
 struct ExecutionContext {
   Algorithms* algorithms;
   GraphHandle graph;
@@ -32,8 +29,7 @@ struct ExecutionContext {
       : algorithms(&algs), graph(gh) {}
 };
 
-// Configuration for FlowPolicy behavior. Mirrors the long-form constructor
-// parameters in a grouped, maintainable struct.
+// Configuration struct for FlowPolicy behavior
 struct FlowPolicyConfig {
   PathAlg path_alg { PathAlg::SPF };
   FlowPlacement flow_placement { FlowPlacement::Proportional };
@@ -51,8 +47,7 @@ struct FlowPolicyConfig {
   double diminishing_returns_epsilon_frac { 1e-3 };
 };
 
-// FlowPolicy orchestrates flow creation, placement, reopt, and removal for a
-// single demand (src,dst,flowClass) on a shared FlowGraph.
+// FlowPolicy manages flow creation, placement, reoptimization for a single demand
 class FlowPolicy {
 public:
   // New config-based constructor
@@ -106,31 +101,30 @@ class FlowPolicy {
   // Core operations
   [[nodiscard]] std::pair<double,double> place_demand(FlowGraph& fg,
                                         NodeId src, NodeId dst,
-                                        std::int32_t flowClass,
+                                        FlowClass flowClass,
                                         double volume,
                                         std::optional<double> target_per_flow = std::nullopt,
                                         std::optional<double> min_flow = std::nullopt);
 
   [[nodiscard]] std::pair<double,double> rebalance_demand(FlowGraph& fg,
                                             NodeId src, NodeId dst,
-                                            std::int32_t flowClass,
+                                            FlowClass flowClass,
                                             double target_per_flow);
 
   void remove_demand(FlowGraph& fg);
 
   [[nodiscard]] const std::unordered_map<FlowIndex, FlowRecord, FlowIndexHash>& flows() const noexcept { return flows_; }
 
-  // Configure static paths to be used for flow creation (if endpoints match).
-  // Each entry is (src, dst, dag, cost). If provided, max_flow_count must be
-  // equal to the number of static paths (or will be set to that number).
+// Configure static paths for flow creation. Each entry is (src, dst, dag, cost).
+// max_flow_count must equal the number of static paths if set.
   void set_static_paths(std::vector<std::tuple<NodeId, NodeId, PredDAG, Cost>> paths);
 
 private:
   // Helpers
   [[nodiscard]] std::optional<std::pair<PredDAG, Cost>> get_path_bundle(const FlowGraph& fg,
                                                           NodeId src, NodeId dst,
                                                           std::optional<double> min_flow);
-  [[nodiscard]] FlowRecord* create_flow(FlowGraph& fg, NodeId src, NodeId dst, std::int32_t flowClass,
+  [[nodiscard]] FlowRecord* create_flow(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass,
                     std::optional<double> min_flow);
   [[nodiscard]] FlowRecord* reoptimize_flow(FlowGraph& fg, const FlowIndex& idx, double headroom);
 
@@ -154,7 +148,7 @@ class FlowPolicy {
   // State
   std::unordered_map<FlowIndex, FlowRecord, FlowIndexHash> flows_;
   Cost best_path_cost_ { std::numeric_limits<Cost>::max() };
-  std::int64_t next_flow_id_ { 0 };
+  FlowId next_flow_id_ { 0 };
 
   // Static paths (optional)
   std::vector<std::tuple<NodeId, NodeId, PredDAG, Cost>> static_paths_;
diff --git a/include/netgraph/core/types.hpp b/include/netgraph/core/types.hpp
@@ -1,4 +1,4 @@
-/* Core type aliases and small helper structs used across the library. */
+/* Core type aliases and helper structs. */
 #pragma once
 
 #include <cstdint>
@@ -14,12 +14,16 @@ using Cap    = double;
 // Semantic alias for flow amounts (same unit as capacity)
 using Flow = double;
 
+// Aliases for flow classification and identifiers for consistency
+using FlowClass = std::int32_t;
+using FlowId = std::int64_t;
+
 // Identity of a flow: endpoints + class (priority bucket) + per-policy unique id
 struct FlowIndex {
   NodeId src;
   NodeId dst;
-  std::int32_t flowClass; // small priority bucket
-  std::int64_t flowId;    // per-policy unique id
+  FlowClass flowClass; // small priority bucket
+  FlowId    flowId;    // per-policy unique id
   friend bool operator==(const FlowIndex& a, const FlowIndex& b) noexcept {
     return a.src==b.src && a.dst==b.dst && a.flowClass==b.flowClass && a.flowId==b.flowId;
   }
@@ -34,8 +38,8 @@ struct FlowIndexHash {
     };
     combine(std::hash<NodeId>{}(k.src));
     combine(std::hash<NodeId>{}(k.dst));
-    combine(std::hash<std::int32_t>{}(k.flowClass));
-    combine(std::hash<std::int64_t>{}(k.flowId));
+    combine(std::hash<FlowClass>{}(k.flowClass));
+    combine(std::hash<FlowId>{}(k.flowId));
     return h;
   }
 };
diff --git a/src/flow_graph.cpp b/src/flow_graph.cpp
@@ -53,7 +53,7 @@ void FlowGraph::remove(const FlowIndex& idx) {
   ledger_.erase(it);
 }
 
-void FlowGraph::remove_by_class(std::int32_t flowClass) {
+void FlowGraph::remove_by_class(FlowClass flowClass) {
   std::vector<FlowIndex> to_rm;
   to_rm.reserve(ledger_.size());
   for (auto const& kv : ledger_) if (kv.first.flowClass == flowClass) to_rm.push_back(kv.first);
diff --git a/src/flow_policy.cpp b/src/flow_policy.cpp
@@ -52,16 +52,16 @@ std::optional<std::pair<PredDAG, Cost>> FlowPolicy::get_path_bundle(const FlowGr
   // Decide whether we need residual-aware SPF and whether to build an edge mask
   const bool require_residual = (sel.require_capacity || (flow_placement_ == FlowPlacement::EqualBalanced && min_flow.has_value()));
   const auto residual = fg.residual_view();
-  std::vector<unsigned char> em; const bool* edge_mask_ptr = nullptr;
+  std::vector<unsigned char> em; std::unique_ptr<bool[]> em_bool; const bool* edge_mask_ptr = nullptr;  // Safe bool array for edge mask
   bool need_mask = false;
   if (require_residual) {
     // In shortest-path EqualBalanced mode, do not enforce per-edge minimum residual at SPF stage
     need_mask = (min_flow.has_value() && !(shortest_path_ && flow_placement_ == FlowPlacement::EqualBalanced));
     if (need_mask) {
-      em.assign(residual.size(), 1u);
+      em_bool.reset(new bool[residual.size()]);
       double thr = *min_flow;
-      for (std::size_t i=0;i<residual.size();++i) em[i] = static_cast<unsigned char>(static_cast<double>(residual[i]) >= thr);
-      edge_mask_ptr = reinterpret_cast<const bool*>(em.data());
+      for (std::size_t i=0;i<residual.size();++i) em_bool[i] = static_cast<double>(residual[i]) >= thr;
+      edge_mask_ptr = em_bool.get();
     }
   }
   SpfOptions opts;
@@ -99,7 +99,7 @@ std::optional<std::pair<PredDAG, Cost>> FlowPolicy::get_path_bundle(const FlowGr
 
 /* Create a new flow using the current path bundle. Returns nullptr if no
    admissible path is available given constraints. */
-FlowRecord* FlowPolicy::create_flow(FlowGraph& fg, NodeId src, NodeId dst, std::int32_t flowClass,
+FlowRecord* FlowPolicy::create_flow(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass,
                               std::optional<double> min_flow) {
   FlowIndex idx{src, dst, flowClass, next_flow_id_++};
   auto pb = get_path_bundle(fg, src, dst, min_flow);
@@ -141,7 +141,7 @@ FlowRecord* FlowPolicy::reoptimize_flow(FlowGraph& fg, const FlowIndex& idx, dou
    Returns (total_placed, leftover). */
 std::pair<double,double> FlowPolicy::place_demand(FlowGraph& fg,
                                                   NodeId src, NodeId dst,
-                                                  std::int32_t flowClass,
+                                                  FlowClass flowClass,
                                                   double volume,
                                                   std::optional<double> target_per_flow,
                                                   std::optional<double> min_flow) {
@@ -301,7 +301,7 @@ std::pair<double,double> FlowPolicy::place_demand(FlowGraph& fg,
    `target_per_flow`. Internally removes and re-places the same total volume. */
 std::pair<double,double> FlowPolicy::rebalance_demand(FlowGraph& fg,
                                                       NodeId src, NodeId dst,
-                                                      std::int32_t flowClass,
+                                                      FlowClass flowClass,
                                                       double target_per_flow) {
   double vol = placed_demand();
   remove_demand(fg);
diff --git a/src/flow_state.cpp b/src/flow_state.cpp

Original file line number	Diff line number	Diff line change
`@@ -53,7 +53,7 @@ void FlowGraph::remove(const FlowIndex& idx) {`
`53`	`53`	`ledger_.erase(it);`
`54`	`54`	`}`
`55`	`55`
`56`		`-void FlowGraph::remove_by_class(std::int32_t flowClass) {`
	`56`	`+void FlowGraph::remove_by_class(FlowClass flowClass) {`
`57`	`57`	`std::vector<FlowIndex> to_rm;`
`58`	`58`	`to_rm.reserve(ledger_.size());`
`59`	`59`	`for (auto const& kv : ledger_) if (kv.first.flowClass == flowClass) to_rm.push_back(kv.first);`