Skip to content

Annotate normal_form_game.py - #576

Open
rht wants to merge 5 commits into
QuantEcon:mainfrom
rht:master
Open

Annotate normal_form_game.py#576
rht wants to merge 5 commits into
QuantEcon:mainfrom
rht:master

Conversation

@rht

@rht rht commented Apr 16, 2021

Copy link
Copy Markdown
Contributor
  • np.ndarray is a catch-all type for all ndarrays regardless of its dtype, and so, is unfortunately less specific than the docstring that specifies ndarray.
  • I'm unsure of the type of action_profile since the functions using it are not documented.
  • I found this bug: normal_form_game.py:921: error: Missing return statement in . There is a case where the function returns None instead of int when the if condition is never satisfied.

@pep8speaks

Copy link
Copy Markdown

Hello @rht! Thanks for opening this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 204:80: E501 line too long (82 > 79 characters)
Line 259:80: E501 line too long (92 > 79 characters)
Line 525:9: E125 continuation line with same indent as next logical line
Line 525:80: E501 line too long (86 > 79 characters)
Line 732:80: E501 line too long (94 > 79 characters)
Line 759:80: E501 line too long (86 > 79 characters)
Line 817:80: E501 line too long (90 > 79 characters)
Line 922:80: E501 line too long (83 > 79 characters)

@rht

rht commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Will fix the line too long issues later.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.003%) to 94.338% when pulling b2a8f98 on rht:master into 6ffbe57 on QuantEcon:master.

@rht

rht commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

There is a case where the function returns None instead of int when the if condition is never satisfied.

There is never such case, from the way payoff_max is specified, but Mypy isn't aware of this. Just ignore the error?

@mmcky

mmcky commented Apr 16, 2021

Copy link
Copy Markdown
Contributor

Just ignore the error?

In Mypy is there a way to add an ignore such as # noqa for coveralls?

@oyamad

oyamad commented Apr 16, 2021

Copy link
Copy Markdown
Member

There is a case where the function returns None instead of int when the if condition is never satisfied.

Yes, it happens when tol < 0. (tol is supposed to be nonnegative, but it is just implicit.)

There are two options:

  1. Just return -1 when the if condition is never satisfied (which happens if and only if tol < 0).
  2. Let tol = 0 if tol < 0.

Maybe option 1?

@rht

rht commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

There is a type: ignore for a single line, but the error appears exactly at the def best_response_2p(...) line. If this line is disabled, the entire function signature's type checking will also be disabled.

@rht

rht commented Apr 16, 2021

Copy link
Copy Markdown
Contributor Author

Just return -1 when the if condition is never satisfied (which happens if and only if tol < 0).

Maybe it is safer to tell the user there is a problem instead of -1? I tried raising an Exception at the end and Mypy no longer complains about missing a return. What should the exception message be?

return Player(payoff_array_new)

def payoff_vector(self, opponents_actions):
def payoff_vector(self, opponents_actions: npt.ArrayLike) -> np.ndarray:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

opponents_actions should have been IntOrArray, which is Union[int, npt.ArrayLike].
But doing so will cause problem in

reduce_last_player(payoff_vector, opponents_actions[i])
. A Union[int, ...] type generally can't be indexed. Alternative solution to my current version is to use cast() before the indexing to cast opponents_actions into an indexable.

Note that int is also part of npt.ArrayLike, since np.array(1) works.

@mmcky

mmcky commented Apr 30, 2021

Copy link
Copy Markdown
Contributor

Just return -1 when the if condition is never satisfied (which happens if and only if tol < 0).

Maybe it is safer to tell the user there is a problem instead of -1? I tried raising an Exception at the end and Mypy no longer complains about missing a return. What should the exception message be?

thanks @rht sorry for my slow response.

tol is supposed to be nonnegative, but it is just implicit

@oyamad should we add an exception if tol is negative instead and then the return is bounded.

@oyamad

oyamad commented Apr 30, 2021

Copy link
Copy Markdown
Member

should we add an exception if tol is negative instead

Actually, this function best_response_2p is for internal use. Negative tol should be handled by the outer function that calls best_response_2p. Let's just return some int, say -1, and write in the docstring, something like "Return -1 if there is no best response action, which occurs when tol < 0".

@oyamad

oyamad commented Apr 30, 2021

Copy link
Copy Markdown
Member

I'm unsure of the type of action_profile since the functions using it are not documented.

  • action_profile in __getitem__ and __setitem__ is int if N=1 and array_like if N>=2.
  • action_profile in is_nash is array_like.

So your annotations are correct.

@rht

rht commented May 1, 2021

Copy link
Copy Markdown
Contributor Author

should we add an exception if tol is negative instead

Actually, this function best_response_2p is for internal use. Negative tol should be handled by the outer function that calls best_response_2p. Let's just return some int, say -1, and write in the docstring, something like "Return -1 if there is no best response action, which occurs when tol < 0".

best_response_2p is currently exported as an external function (

from .normal_form_game import pure2mixed, best_response_2p
) and is not used internally anywhere in the module.

@mmcky

mmcky commented Apr 5, 2022

Copy link
Copy Markdown
Contributor

@oyamad are you happy with these annotations?

@oyamad oyamad self-assigned this Apr 5, 2022
@rht

rht commented Apr 5, 2022

Copy link
Copy Markdown
Contributor Author

I think it is safe to replace all of IntOrArrayT with npt.ArrayLike? The code selection part that checks whether actions is a scalar or array happens in

if isinstance(action, numbers.Integral): # pure action
return payoff_array.take(action, axis=-1)
else: # mixed action
return payoff_array.dot(action)
.

@mmcky
mmcky requested a review from Copilot August 16, 2025 01:06

This comment was marked as outdated.

@coveralls

coveralls commented Aug 16, 2025

Copy link
Copy Markdown

Coverage Status

coverage: 92.629% (+0.003%) from 92.626%
when pulling 0c16700 on rht:master
into 6045b51 on QuantEcon:main.

@mmcky

mmcky commented Aug 20, 2025

Copy link
Copy Markdown
Contributor

@copilot can you please review comments by @rht above.

@mmcky
mmcky requested a review from Copilot August 20, 2025 22:11

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 pull request adds comprehensive type annotations to the normal_form_game.py module in the QuantEcon library. The PR addresses type safety by adding explicit type hints throughout the module and fixes a missing return statement bug.

Key changes:

  • Added comprehensive type annotations to all classes, methods, and functions
  • Introduced a custom type alias IntOrArrayT for flexibility with int/array parameters
  • Fixed a missing return statement bug in the best_response_2p function

def best_response_2p(payoff_matrix, opponent_mixed_action, tol=1e-8):
def best_response_2p(
payoff_matrix: np.ndarray, opponent_mixed_action: np.ndarray, tol: float = 1e-8
) -> int:

Copilot AI Aug 20, 2025

Copy link

Choose a reason for hiding this comment

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

The function best_response_2p is missing a return statement implementation. The function signature indicates it should return an int, but there's no function body shown that would ensure this return type is always satisfied.

Copilot uses AI. Check for mistakes.
Comment thread quantecon/game_theory/normal_form_game.py Outdated

"""
def __init__(self, data, dtype=None):
def __init__(self, data: npt.ArrayLike, dtype: Optional[np.dtype] = None):

Copilot AI Aug 20, 2025

Copy link

Choose a reason for hiding this comment

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

The parameter data is annotated as npt.ArrayLike but the implementation shows it can also accept an array of Player objects. The type annotation should be Union[npt.ArrayLike, Sequence[Player]] to accurately reflect all accepted input types.

Suggested change
def __init__(self, data: npt.ArrayLike, dtype: Optional[np.dtype] = None):
def __init__(self, data: Union[npt.ArrayLike, Sequence[Player]], dtype: Optional[np.dtype] = None):

Copilot uses AI. Check for mistakes.
rht and others added 3 commits August 21, 2025 08:24
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@rht

rht commented Aug 21, 2025

Copy link
Copy Markdown
Contributor Author

Latest changes:

  1. Addressed Annotate normal_form_game.py #576 (comment)
  2. Accidentally committed a Ruff formatted file (that converted ' to ").
  3. Modernized the annotation to a newer Mypy standard.

@mmcky

mmcky commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Status update after a fresh review (2026-08-14): this PR is now gated on the type-hint policy discussion in #897 — annotating a single module without a repo-wide policy, a py.typed marker, and a checked mypy job gives little benefit, and unchecked annotations tend to drift. Once #897 is decided we will either ask for a clean rebase here or close with thanks.

Two notes on the current diff for whenever that decision lands:

  1. The return -1 fix for best_response_2p that was agreed in the 2021 discussion above is no longer in the diff — the current revision only annotates the signature -> int while the body can still fall through to None. That fix has been extracted into BUG: Return -1 from best_response_2p instead of falling through to None #936 so it can land independently.
  2. Roughly half the diff is Ruff formatting churn (quote conversion and line rewrapping) that was committed accidentally per the comment above — a future rebase should drop that so the diff is annotations only.

Thanks @rht for the patience on this one — the honest answer is that the repo needs to make the policy call in #897 first, and we will close the loop here either way.

mmcky added a commit that referenced this pull request Aug 16, 2026
…ne (#936)

* BUG: Return -1 from best_response_2p instead of falling through to None

When tol < 0, no action satisfies the tolerance condition and the
function fell through to an implicit None despite documenting an int
return. Return -1 in that case, as suggested by oyamad in the review
discussion on #576, and document the behavior.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* DOC: Drop 'must be nonnegative' from best_response_2p tol docs

The Returns section already documents the -1 sentinel for tol < 0, so
the input restriction read as contradictory. Addresses Copilot review
feedback on #936.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* DOC: Frame the -1 return of best_response_2p as an error condition

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* DOC: Restore the nonnegativity stipulation for best_response_2p tol

With -1 now explicitly framed as an error condition in the Returns
section, stating the valid domain in the parameter docs is
complementary rather than contradictory: the parameter doc gives the
precondition, the Returns doc gives the failure mode.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

7 participants