Skip to content

Fix/magic numbers duplication - #18

Merged
Codex-Crusader merged 3 commits into
Codex-Crusader:mainfrom
Sahilraj3107:fix/magic-numbers-duplication
Sep 16, 2025
Merged

Fix/magic numbers duplication#18
Codex-Crusader merged 3 commits into
Codex-Crusader:mainfrom
Sahilraj3107:fix/magic-numbers-duplication

Conversation

@Sahilraj3107

Copy link
Copy Markdown
Contributor

Found that both MCTS.run() and MCTS._simulate() and created the PROMOTION_MAP constant also defined at the top of the file.

What I changed:

Removed duplicate promo_map dictionaries from both methods

Now both use PROMOTION_MAP.get(mv.promotion, 0) instead

Reduces code duplication and makes it easier to maintain

And the test for the above is included in test_promotion_mapping.py in tests folder

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR eliminates code duplication by consolidating promotion mapping logic into a single constant. The duplicate promotion mapping dictionaries in three different methods (MCTS.run(), MCTS._simulate(), and self_play_episode()) are replaced with a shared PROMOTION_MAP constant defined at the top of the file.

  • Defines a global PROMOTION_MAP constant containing chess piece promotion mappings
  • Removes duplicate promotion mapping dictionaries from three methods
  • Updates all methods to use the shared constant with .get() method calls

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
azlite_portfolio_clean.py Adds global PROMOTION_MAP constant and removes duplicate dictionaries from three methods
tests/test_promotion_mapping.py Adds comprehensive tests for the new PROMOTION_MAP constant and its usage

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread azlite_portfolio_clean.py Outdated
Comment thread azlite_portfolio_clean.py
promo_map = {chess.QUEEN: 1, chess.ROOK: 2,
chess.BISHOP: 3, chess.KNIGHT: 4}
promo_idxs.append(promo_map.get(mv.promotion, 0))
PROMOTION_MAP.get(mv.promotion, 0)

Copilot AI Sep 16, 2025

Copy link

Choose a reason for hiding this comment

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

Line 348 calls PROMOTION_MAP.get(mv.promotion, 0) but doesn't use the result. This appears to be a copy-paste error - line 348 should be removed as line 349 correctly appends the value to the list.

Suggested change
PROMOTION_MAP.get(mv.promotion, 0)

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@Codex-Crusader

Copy link
Copy Markdown
Owner

Lint error, merge not possible unless
line under 80 charecters
Line 28
Line 348

Affected areas

@Codex-Crusader

Copy link
Copy Markdown
Owner

Manual Changes recommended

@Codex-Crusader Codex-Crusader left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

merge after optionally removing the unused line in _simulate. This is a safe and beneficial change.
But merging request will only be approved after check successfulness has been verified

@Codex-Crusader
Codex-Crusader merged commit 27f4b37 into Codex-Crusader:main Sep 16, 2025
1 check failed
@Codex-Crusader

Copy link
Copy Markdown
Owner

Approval done and fixed. Thank you for your work

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