GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-43475: Fix worst case collision behavior for NaN instances by rhettinger · Pull Request #25493 · python/cpython

Conversation

@rhettinger

@rhettinger rhettinger commented Apr 21, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue43475

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@rhettinger
bpo-43475: Fix worst case collision behavior for NaN instances
b9cde10
@rhettinger rhettinger added the performance Performance or resource usage label Apr 21, 2021
@rhettinger
rhettinger requested review from mdickinson and tim-one April 21, 2021 03:13
@rhettinger
rhettinger requested a review from tiran as a code owner April 21, 2021 03:13
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Apr 21, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Apr 21, 2021
@rhettinger
Fix decimal and docs as well
20b73c3
@rhettinger
rhettinger removed the request for review from tiran April 21, 2021 03:38
@rhettinger
Add casts
056a4f7
@rhettinger rhettinger added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@bedevere-bot

bedevere-bot commented Apr 21, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @rhettinger for commit 056a4f7 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
rhettinger added 2 commits April 21, 2021 07:50
@rhettinger
Use existing pointer hash logic
dc73c3c
@rhettinger
Improve comment wording
9f3f9e9
@rhettinger rhettinger added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@bedevere-bot

bedevere-bot commented Apr 21, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @rhettinger for commit 9f3f9e9 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@rhettinger
Fix typo in comment
6ac6975
@rhettinger
rhettinger merged commit a07da09 into python:master Apr 22, 2021
@bedevere-bot bedevere-bot removed the awaiting core review label Apr 22, 2021
@realead

realead commented Jun 10, 2021

Copy link
Copy Markdown

Is my understanding right, that this PR would break the following code:

import math

class A:
    def __init__(self, a):
        self.a=a
    def __hash__(self):
        return hash(self.a)
    def __eq__(self, other):
        if(math.isnan(self.a) and math.isnan(other.a)):
            return True
        return self.a == other.a
    def __repr__(self):
        return str(self.a)
        
set([A(float("nan")), A(float("nan"))])  # result: {nan}

I.e. when somebody tries to wrap Float and change __eq__ in such a way, that all float-nans will be equivalent?

With this PR, the chances are high, that the result will be {nan, nan}, as hashes from both objects will be different.

Until now, it was clear - don't put nans into set/dict because the default "="-relation for floats isn't an equivalence relation. People worked around this by redefining the "="-relation and didn't so for hash function because until now "a,b - nans => hash(a)=hash(b)" was given.


I think the intuitive behavior for set([float("nan"), float("nan")] is {nan} and not {nan, nan}. Given how Py_EQ is defined for floats, this is not possible. Maybe there is need for a new Py_EQ_FOR_HASH comparator, which would be used in hashset/hashdict and be more or less the same as Py_EQ but would yield true for nan==nan.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mdickinson mdickinson mentioned this pull request Jun 14, 2021
bpo-43475: Add what's new entry for NaN hash changes #26725
Merged
@stuartarchibald stuartarchibald mentioned this pull request Nov 8, 2021
Exp/3.10+new with+np121 numba/numba#7544
Closed
@gvanrossum gvanrossum mentioned this pull request Dec 20, 2023
Add Py_HashDouble() function capi-workgroup/decisions#2
Closed
4 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mdickinson mdickinson Awaiting requested review from mdickinson
@tim-one tim-one Awaiting requested review from tim-one

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

performance Performance or resource usage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@rhettinger @bedevere-bot @realead @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.