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

Edge Cost Query #94

Merged
merged 42 commits into from
Jan 30, 2025
Merged

Edge Cost Query #94

merged 42 commits into from
Jan 30, 2025

Conversation

coliem
Copy link

@coliem coliem commented Jan 27, 2025

Closes #65 #90

  • Implemented C++ methods CountEdges and CountEdgesFromEdgeSets to count the number of edges of given cost type within the graph.
  • Implemented C++ methods GetEdgeCosts to get all edge costs of given cost type within a graph. A new method GetCostMap that exposes the private attribute for the graph's cost map to public is utilized for this task. If only a subset of edge costs are needed, then the method GetEdgeCostFromNodeIDs is used instead. The user passes in an array of edges using the ids of the attached nodes (parent1, child1) and gets all costs of desired edges.
  • Implemented C++ method AlternateCostsAlongPath which takes a path and computes a given cost type of each edge in the path. This uses helper function MapPathToVectorOfNodes to map a vector of format [node1,node2,...,nodek] to [node1,node2,node2,...,nodek] to utilize the GetEdgeCosts methods.
  • C_Interface functions GetEdgeCosts, which gets all costs of a given type, and GetEdgeCostsFromNodeIDs, which gets the costs of a subset of edges represented by node ids.
  • C_Interface function CountNumberOfEdges which counts the number of edges of a given cost type within the graph.
  • C_Interface function AlternateCostsAlongPathWithIDs and AlternateCostsAlongPathStruct which sends the correspondingg C++ methods to Python.
  • Python/C GetEdgeCosts and NumEdges call the new C_Interface functions to query cost type and count number of edges.
  • Python/C AlternateCostsAlongPath calls the corresponding C_Interface function to compute alternate costs along a path. It is possible to compute alternate costs of a path in the form of a list of ids, but support for path structures still need to be implemented.
  • Python SizeOfEdgeVector to compute the size of an edge vector in C++.
  • Python DestroyEdges to destroy a list of edges in C++.
  • DocTests added for Python functions
  • New example PathQuerying.py which guides the user on how to get a shortest path and how to get the alternate costs for nodes along that path. Provides plots to help the user visualize the process.
  • Debugged AddEdgeToGraph Python implementation which took a node yet couldn't handle it properly. A new C_Interface function AddEdgeFromNodeStructs was added for this purpose and AddEdgeToGraph now works as intended.
  • EdgeStruct Python implementation which resembles the Edge structure (not IntEdge) in C++. This comes along with the EdgeList class and the CreateListOfEdgeStructs method.
  • Implemented GetEdgesForNode in Python and C++, which takes in a node and a graph and finds all edges corresponding to that node. The Python implementation returns an EdgeList containing all the edges.
  • Added UnitTests corresponding to each of these functions.

Attached is a screenshot of all the VS test cases (including three new test cases for the C++ and C_Interface functions, with the exception of HFUnitTests/CInterfaceTests/C_Pathfinder/CreateManyPathsPerformanceTest due to runtime issues) and the test_graph.py test cases passing.
image
image

@coliem coliem marked this pull request as draft January 27, 2025 23:59
@coliem coliem marked this pull request as ready for review January 28, 2025 22:05
out_scores[i] = v_costs[i];
}
*out_score_size = v_costs.size();
//if (!std::isfinite(*out_float)) *out_float = -1.0f;
Copy link
Owner

Choose a reason for hiding this comment

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

Reason?

Copy link
Author

Choose a reason for hiding this comment

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

Removed. It was a reminder that invalid edges come out as NaN which isn't finite.

\brief Get the alternate costs of traversing a given path

\param g The graph to traverse
\param path A path to get costs from
Copy link
Owner

Choose a reason for hiding this comment

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

is this common in other docs? it says path but the parameter is ids

Copy link
Author

Choose a reason for hiding this comment

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

I mixed up the two docstrings between AlternateCostsAlongPathWithIDs and AlternateCostsAlongPathStruct. Should be resolved and other docs are fine

vector<float> Graph::GetEdgeCostsFromNodeIDs(vector<int>& ids, const string& cost_type) const {
// Assume that ids is given in [parent1, child1, parent2, child2,...]
// Return an empty array if this attribute doesn't exist
// Need to raise error if odd number of ids
Copy link
Owner

Choose a reason for hiding this comment

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

Do the check of odd ids.

there should be a unittest for testing what happens when invalid edge is given. maybe handled in GetCost

Copy link
Author

Choose a reason for hiding this comment

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

For inputs with an odd number of ids, the C++ function GetEdgeCostsFromNodeIDs now throws std::invalid_argument and the C_Interface throws GENERIC_ERROR if this is the case (I didn't want to add any new error codes). Moreover, NaN represents an invalid edge, which still gets added to the output array. If you want this to throw an error, then I can modify the code.

The corresponding UnitTest has been updated to check these two edge cases.

src/Cpp/tests/src/SpatialStructures.cpp Show resolved Hide resolved
src/Python/dhart/pathfinding/shortest_path.py Outdated Show resolved Hide resolved
src/Python/dhart/spatialstructures/graph.py Show resolved Hide resolved

def DestroyEdges(edge_list_ptr: c_void_p):
""" Call the destructor for a list of edges """
HFPython.DestroyEdges(edge_list_ptr)
Copy link
Owner

Choose a reason for hiding this comment

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

Did you make a list of edges?

Copy link
Author

Choose a reason for hiding this comment

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

No, but the C_Interface function was already implemented so I just used that.

return out_vals

# Extract data from out_scores
return list(out_scores[:out_scores_size.value])
Copy link
Owner

Choose a reason for hiding this comment

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

why is this cast to a list?

Copy link
Author

Choose a reason for hiding this comment

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

It was the same concept as the for loop. I didn't really know what you were talking about at the time and I will replace it with the raw data when I fix all of the for loops.

elif error_code == HF_STATUS.OUT_OF_RANGE:
# Tried to add an edge to an alternate cost type
# that didn't exist
raise InvalidCostOperation(
Copy link
Owner

Choose a reason for hiding this comment

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

Did you test if any of these error codes are actually hit?

@@ -483,7 +502,7 @@ def GetEdgeCosts(self, cost_type: str, ids: List[int] | None = None) -> List[flo
>>> g.AddEdgeToGraph(0,1,50)
>>> g.AddEdgeToGraph(0,2,50)
>>> g.AddEdgeToGraph(1,2,50)
>>> csr = g.CompressToCSR()
>>> _ = g.CompressToCSR() #doctest: +ELLIPSIS
Copy link
Owner

Choose a reason for hiding this comment

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

I have wondered about this. The does the C function checks if the graph was compressed already? Historically we have said you always need to compress, and all of these examples do it, but what if not?

Copy link
Author

Choose a reason for hiding this comment

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

If you're talking about CompressToCSR(), the C++ function checks if it was compressed before doing anything. The graph has a variable needs_compression that is maintained.

void Graph::Compress() {
// Only do this if the graph needs compression.
if (needs_compression) {
// If this has cost arrays then we never should have come here
assert(!this->has_cost_arrays);
// Note that the matrix must have atleast one extra row/column
int array_size = this->size() + 1;
// Resize the edge matrix
ResizeIfNeeded();
// Set the edge matrix from triplets.
edge_matrix.setFromTriplets(triplets.begin(), triplets.end());
// Mark this graph as not requiring compression
needs_compression = false;
}
}

@cadop cadop merged commit c5d3d52 into cadop:dev Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants