-
Notifications
You must be signed in to change notification settings - Fork 4
add the Unit-Disk Mapping Module #125
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you change the structure of your code first?
qamomile/core/ising_qubo.py
Outdated
J = np.zeros((n, n)) | ||
h = np.zeros(n) | ||
|
||
for (i, j), value in self.ising_model.quad.items(): | ||
J[i, j] = value | ||
J[j, i] = value | ||
|
||
for i, value in self.ising_model.linear.items(): | ||
h[i] = value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there big reasons to create matrix version of Ising model?
If so, I think it is better to use this instead of your own implementation.
class IsingMatrix: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I use IsingMatrix
Class instead. Create the numpy matrix is easier for using scipy solver.
qamomile/core/ising_qubo.py
Outdated
|
||
Args: | ||
use_brute_force: Whether to use brute force enumeration for small problems | ||
binary_variables: Whether to use {0,1} variables (True) or {-1,1} variables (False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in Qamomile workflow, library only deals with Ising model (spin variables), so you need to care about {0,1} case.
Or if binary {0,1} is better than spin {-1,1} when converting, you can only deal with better formulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I removed the {0,1} encoding option. It was for other type of problem.
if isinstance(indices, tuple) and len(indices) == 2: | ||
i, j = indices | ||
|
||
if not (1 <= i <= self.height and 1 <= j <= self.width): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i
and j
start from 1, not 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was easier to compare to the original Julia version. There julia use 1-indexed array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also add a short note in the class or method docstring to indicate that indexing is 1-based.
qamomile/core/ising_qubo.py
Outdated
import numpy as np | ||
from ..udm import map_qubo, solve_qubo, qubo_result_to_networkx, QUBOResult |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think udm
is independent subpackage, so it should not use in core
.
It is better to move this to inside of udm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Now the entire udm related files are outside of the core. I created a Ising_UnitDiskGraph
in the udm module instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few points should be addressed. I’ve included inline comments with suggestions.
let me know if you'd like help implementing any of them.
qamomile/udm/__init__.py
Outdated
""" | ||
|
||
|
||
from .dragondrop import ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module name dragondrop
doesn't clearly show its purpose or functionality. If it's related to solving QUBO problems or MWIS on graphs, a name like graph_solver
, qubo_tools
, or mwis_utils
might be more appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed from dragondrop
do mwis_solver
if isinstance(indices, tuple) and len(indices) == 2: | ||
i, j = indices | ||
|
||
if not (1 <= i <= self.height and 1 <= j <= self.width): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might also add a short note in the class or method docstring to indicate that indexing is 1-based.
qamomile/udm/core.py
Outdated
SHOW_WEIGHT = False | ||
|
||
# ONE type - a singleton to represent the constant 1 for unweighted cases | ||
class ONE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider renaming ONE
to something more descriptive like UnweightedMarker
or UnweightedSingleton
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed from ONE to UNWEIGHTED
"""Get all vertex indices.""" | ||
return range(self.num_vertices()) | ||
|
||
def cell_matrix(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In .cell_matrix
, it may be safer to add a bounds check to ensure that node.loc
is still in the grid size, to prevent index errors if invalid nodes are passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added following bound checks:
for node in self.nodes:
i, j = node.loc
assert 0 <= i < self.size[0], f"i={i} is out of bounds [0, {self.size[0]})"
assert 0 <= j < self.size[1], f"j={j} is out of bounds [0, {self.size[1]})"
matrix[i][j] = SimpleCell(occupied=True, weight=node.weight)
qamomile/udm/utils.py
Outdated
return (loc[1], loc[0]) | ||
|
||
# Apply transformations with a center | ||
def apply_transform(loc: Tuple[int, int], center: Tuple[int, int], transform_func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest adding type hints to apply_transform for clarity. For example:
def apply_transform(loc: Tuple[int, int], center: Tuple[int, int], transform_func): | |
def apply_transform(loc: Tuple[int, int], center: Tuple[int, int], transform_func: Callable[[Tuple[int, int]], Tuple[int, int]]) -> Tuple[int, int]: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied your suggestions
qamomile/udm/utils.py
Outdated
|
||
return True, diff if diff is not None else 0 | ||
|
||
def is_unit_disk_graph(grid_graph) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a verbose=Fals
e argument to is_unit_disk_graph
, so if a mismatch is found, it can print the problematic pair for easier debugging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a verbose option:
def is_unit_disk_graph(grid_graph, verbose=False) -> bool:
# ---------------------------omitted---------------------------------
# If there's a mismatch, this is not a valid unit disk graph
if should_be_connected != is_connected:
if verbose: print(f"({i}, {j}) should be connected but is not")
return False
qamomile/udm/utils.py
Outdated
def simple_graph_from_edgelist(edgelist: List[Tuple[int, int]]) -> nx.Graph: | ||
"""Create a simple graph from an edge list.""" | ||
g = nx.Graph() | ||
for i, j in edgelist: | ||
g.add_edge(i, j) | ||
return g |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function simple_graph_from_edgelist can be simplified using:
def simple_graph_from_edgelist(edgelist: List[Tuple[int, int]]) -> nx.Graph: | |
"""Create a simple graph from an edge list.""" | |
g = nx.Graph() | |
for i, j in edgelist: | |
g.add_edge(i, j) | |
return g | |
def simple_graph_from_edgelist(edgelist: List[Tuple[int, int]]) -> nx.Graph: | |
"""Create a simple graph from an edge list.""" | |
g = nx.Graph() | |
return nx.Graph(edgelist) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
qamomile/udm/dragondrop.py
Outdated
integrality = np.ones(n, dtype=bool) | ||
|
||
# Solve the MILP | ||
res = milp(c=c, constraints=constraints, bounds=bounds, integrality=integrality) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
solve_mwis_scipy
should check res.success
to ensure the MILP solver didn't fail such like the case res.success == False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added solver status message block:
# Solve the MILP
res = milp(c=c, constraints=constraints, bounds=bounds, integrality=integrality)
# Check solution status
if not res.success:
status_messages = {
1: "Iteration or time limit reached",
2: "Problem is infeasible",
3: "Problem is unbounded",
4: f"Other error: {res.message}"
}
error_msg = status_messages.get(res.status, f"Unknown error: {res.message}")
raise RuntimeError(f"Failed to find optimal solution: {error_msg}")
# The solution vector res.x contains the binary decisions.
selected_nodes = [node for node, idx in node_to_idx.items() if round(res.x[idx]) == 1]
return selected_nodes, -res.fun, res.x
qamomile/udm/dragondrop.py
Outdated
gg, pins = post_process_grid(grid, weights, np.zeros_like(weights)) | ||
|
||
# Calculate overhead | ||
mis_overhead = (n - 1) * n * 4 + n - 4 - 2 * graph.number_of_edges() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a short docstring to explain fomulation of overhead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this overhead calculation is for a general mwis instance, which is not fully implmented/tested yet. So comment this line out for now.
The added module udm is integrated to Qamomile in the below structure:
The updated
Qamomile
package is capable of:scipy/milp
to find MWIS solutionsBloqade
orPulser
Emulators to solve MWIS-UDG problem.