Skip to content

Conversation

@OrestisLomis
Copy link
Contributor

PR for the sudoku variants I made some time ago. I tried some of these with LLMs as well and they quite difficult so perhaps interesting as projects for the course as well? I also noticed a small mistake in the chess recognition example.

Copy link
Contributor

@hbierlee hbierlee left a comment

Choose a reason for hiding this comment

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

Main thoughts:

  • Obviously, I'm not checking the constraints in depth. But if there is a unique solution, adding an assert will convince me that it's correct.
  • Plus, if solved in <5 seconds, can these examples tested by test_examples?
  • CI is failing still
  • Also I think you meant to request me for review, not assign me. The assigned person is the one doing the coding.

(Count(board, n) <= 2) &
(Count(board, q) <= 1)
(
(Count(board, b) + Count(board, r) + Count(board, n) + Count(board, q) <= 2+2+1+1 + 8-Count(board, p))
Copy link
Contributor

Choose a reason for hiding this comment

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

fine for me to include in this PR, but normally this could be split up in separate PRs (although it's a little more effort)

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd update the comment to reflect the new/fixed constraint

import numpy as np
import time

# This CPMpy example solves a sudoku by KNT, which can be found on https://logic-masters.de/Raetselportal/Raetsel/zeigen.php?id=0009KE
Copy link
Contributor

Choose a reason for hiding this comment

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

if there's a copy-paste description, you could add it. These comments can become nice parts of the documentation if you add them as doc-strings at the top of the module/file (see e.g. chess-recognition)



def killer_cage(idxs, values, regions, total):
# the sum of the cells in the cage must equal the total
Copy link
Contributor

Choose a reason for hiding this comment

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

I can recommend getting in the habit to make these comments also into doc-strings

# the sum of the cells in the cage must equal the total
# all cells in the cage must be in the same region
constraints = []
constraints.append(cp.sum([values[r, c] for r,c in idxs]) == total)
Copy link
Contributor

Choose a reason for hiding this comment

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

just for fun, I've been trying more numpy-type features lately. Could this become cp.sum(values[idxs]) == total?

print("The regions are:")
print(cell_regions.value())
print("The region cardinals are:")
print(region_cardinals.value()) No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

for all of these, a good assert is to add the (unique?) solution at the bottom, and check that value matches it. that way, constraints can be simply changed/improved and you immediately know whether the solution is still correct.

if (r,c) == (2,8):
# The path starts on emitter. It doesn't have any previous cells so the first 3 vectors in the sequence must be fully -1. As for the the last 3, they are taken from the neighbour. Vice versa also applies.
constraints.append(cp.all([cp.all(sequence[r,c,:3].flatten() == -1)]))
constraints.append(cp.any([cp.all([path[nr,nc] == 2, sequence[r,c,3,0] == cells[nr,nc], sequence[r,c,4,0] == nr, sequence[r,c,5,0] == nc, cp.all(sequence[r,c,3,1:] == sequence[nr,nc,3,:19]), cp.all(sequence[r,c,4,1:] == sequence[nr,nc,4,:19]), cp.all(sequence[r,c,5,1:] == sequence[nr,nc,5,:19]), sequence[nr,nc,0,0] == cells[r,c], sequence[nr,nc,1,0] == r, sequence[nr,nc,2,0] == c, cp.all(sequence[nr,nc,0,1:] == sequence[r,c,0,:19]), cp.all(sequence[nr,nc,1,1:] == sequence[r,c,1,:19]), cp.all(sequence[nr,nc,2,1:] == sequence[r,c,2,:19])]) for nr, nc in neighbours]))
Copy link
Contributor

Choose a reason for hiding this comment

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

if these things represent some kind of path, could this path be kept in a separate data structure to make this somewhat more insightfull?

Other way to make this somewhat more readable is to use an auto-formatter like ruff on just this files. It will introduce a lot of newlines, but it will be much easier to read and edit.

import cpmpy as cp
import numpy as np

from cpmpy.tools.explain import optimal_mus, mus
Copy link
Contributor

Choose a reason for hiding this comment

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

unused imports?

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.

3 participants