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

[Chess] Accelerate chess_utils.py import (jnp => np) #1170

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

Akulen
Copy link
Contributor

@Akulen Akulen commented Feb 13, 2024

Importing _src/chess_utils.py currently takes between 10 and 15 seconds.
It is mainly due to the fact that all pre-computations are made on jax.numpy array, which are really slow.
This PR reduces the import time to ~1s, by using numpy array instead, and converting them to jax.numpy array at the end.

@sotetsuk
Copy link
Owner

Thank you for a PR! I'll check it in a few days.

@Akulen
Copy link
Contributor Author

Akulen commented Mar 4, 2024

Did the same modification to gardner_chess. The time gain is less impressive, but it still goes from 3-4s to <1s

# usage: CAN_MOVE[piece, from_x, from_y]
# CAN_MOVE[0, :, :] are all -1
# Note that the board is not symmetric about the center (different from shogi)
# You can imagine that the viewpoint is always from the white side.
# Except PAWN, the moves are symmetric about the center.
# We define PAWN as a piece that can move up, down, and diagonally, and filter it according to the turn.
# We define PAWN as a piece that can move up and diagonally.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why, but it seems that for gardner chess, this comment was previously untrue, as the following condition (line 56) is

if r1 - r0 == 1 and np.abs(c1 - c0) <= 1:

instead of

if np.abs(r1 - r0) == 1 and np.abs(c1 - c0) <= 1:

Copy link
Owner

Choose a reason for hiding this comment

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

This will be investigated in #1174
Thank you for your comment!

@sotetsuk
Copy link
Owner

Hi! I'm so sorry for late response 🙏
I locally confirmed it passes tests and actually improve speed! Great thanks! ❤️
CI failed in different part. I'll fix it in different PR. I'll merge this PR.
Again thank you so much for your effort and sorry for late response 🙏

@sotetsuk sotetsuk merged commit 262afea into sotetsuk:main Mar 13, 2024
1 of 2 checks passed
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