Repository navigation
Keep the first maximum on ties and unordered comparisons - #231
Apeksha9389 wants to merge 3 commits into
Conversation
|
Could a maintainer approve the pending CI runs and review this PR for merging? Local tests, Flake8, Black, MyPy, and PyRight all pass. Thank you. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
maxfischer2781
left a comment
maxfischer2781
left a comment
There was a problem hiding this comment.
Minor query on the implementation. I'm mostly confused by the test style.
| is_better = best < item if invert else item < best | ||
| if is_better: |
There was a problem hiding this comment.
Any reason why the boolean is factored out of the comparison instead of being in-line?
There was a problem hiding this comment.
Updated in 6a3caf0: comparisons are now inline, the test parametrizes actual key callables, and input selection is separate from the builtin/asyncstdlib assertion. Using distinct float objects removes the custom ordering class and label logic, including the CodeQL warning. Added [2.0, nan, 3.0] to verify replacement still occurs after an unordered value. All 413 tests, Flake8, Black, MyPy and PyRight pass locally.
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("key_kind", ["none", "sync", "async"]) |
There was a problem hiding this comment.
Any reason for using str settings that are resolved in the function instead of directly providing keys?
| assert actual is expected | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("values", [[float("nan"), 3.0], [3.0, float("nan"), 2.0]]) |
There was a problem hiding this comment.
Please also check the case that replacement is necessary after the unordered value, i.e. [2.0, float("nan"), 3.0].
| Record(1, "low"), | ||
| Record(3, "first"), | ||
| Record(3, "second"), | ||
| Record(2, "tail"), |
There was a problem hiding this comment.
This looks like way more logic than it actually contains. Please use "low"/"high" or "head"/"tail" pairs instead of mixing them. Or just outright go with "first" for any unique priority as well.
| expected = max(records, key=key) | ||
| actual = await a.max( | ||
| asyncify(records) if async_input else records, | ||
| key=awaitify(key) if key_kind == "async" else key, | ||
| ) | ||
| assert actual is expected |
There was a problem hiding this comment.
This should easily fit into one line if it isn't folded with input selection logic. As per most tests, both parts of the "builtin is asyncstdlib" pair should be built in the assert statement.
| @pytest.mark.parametrize("async_key", [False, True]) | ||
| @pytest.mark.parametrize("async_input", [False, True]) |
There was a problem hiding this comment.
The comment on str settings also applies to these bool settings. Any reason why you use them instead of directly providing the keys/input?
| @pytest.mark.parametrize("async_input", [False, True]) | ||
| @sync | ||
| async def test_max_keeps_first_equal_item(key, async_key, async_input): | ||
| values = [float(value) for value in (1, 3, 3, 2)] |
There was a problem hiding this comment.
I don't understand why you removed the helper class, it seemed perfectly servicable.
Note that float identity is an implementation detail. Compare:
Python 3.12.15 (d90944c94ad6, Oct 14 2026, 09:15:46)
[PyPy 8.0.1 with GCC Apple LLVM 21.0.0 (clang-2100.3.34.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>>> (a:=[float(value) for value in (3, 3)])[0] is a[1]
True
Python 3.12.13 (main, Mar 3 2026, 12:39:30) [Clang 17.0.0 (clang-1700.6.4.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> (a:=[float(value) for value in (3, 3)])[0] is a[1]
False
Why did you switch from the helper class to float?
| ) | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("key", [None, lambda item: item.real]) |
There was a problem hiding this comment.
Since you are relying on float.real to preserve identity, can you point me to the specification for this? I wasn't even able to find the method in the language spec.
There was a problem hiding this comment.
I do not have a specification guaranteeing either distinct float identity or identity preservation by float.real. I switched to floats to avoid the helper class’s incomplete-ordering warning, but that weakened the test on PyPy.
Restored explicit record instances in e731571 and used total_ordering with value-based eq and lt. The test now takes the actual iterable constructors and key callables directly, including an async key, without bool selectors or float.real. All six tied-record cases fail with the original max comparator restored. With the fix, all 411 tests, Black, Flake8, MyPy and PyRight pass locally on CPython 3.12. I have not run PyPy locally.
|
See #230 (comment) |
3 participants
This PR fixes
max()returning the last tied item instead of the first. With records of equal priority, the current implementation replaces the saved record whenever the new record is not smaller, including ties. It can also replace a valid maximum with an unordered NaN.Use a strictly better comparison in the appropriate direction, with and without a key function. This preserves the first tied record as Python's
max()does, while retaining the existing minimum behavior and one key evaluation per item.Regressions cover tied record identity for synchronous/asynchronous inputs, no key, synchronous keys and asynchronous keys, plus NaN before and after an ordinary maximum. All six tied-record cases fail with the original comparator restored and pass here. Explicit record instances with total ordering avoid depending on float identity across interpreters.
Validation on Python 3.12: 411 tests passed, Flake8 and Black passed, MyPy and PyRight reported no issues. Other interpreters are left to CI.
Prepared with AI assistance; reproduced and tested locally.
Closes #235