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

[Improvement] Public deterministic hash method for RegisterLayout #490

Closed
Augustinio opened this issue Mar 28, 2023 · 2 comments · Fixed by #492
Closed

[Improvement] Public deterministic hash method for RegisterLayout #490

Augustinio opened this issue Mar 28, 2023 · 2 comments · Fixed by #492
Milestone

Comments

@Augustinio
Copy link
Collaborator

RegisterLayout's __hash__ method is not idempotent as it is defined as:

def __hash__(self):
    return hash(self._safe_hash()) 

The _safe_hash() method is idempotent but is private and returns a byte string.

For our public APIs, if we want to make use of public methods from Pulser there is currently only one way to target a RegisterLayout from a Device, which is to rebuild a whole RegisterLayout instance (passed from the API) and check for equality with each of the layouts registered on the device.

We would like a deterministic, idempotent, public method on RegisterLayout that returns an int or hexstring. Which we could then easily add as path param to our delete method to remove a layout from a device.

We could for example add the following method:

def __index__(self) -> int:
        return int(self._safe_hash().hex(), 16)

which could then allow us to call hex on an instance of RegisterLayout.

@HGSilveri
Copy link
Collaborator

This sounds good to me, I just have an objections against using __index__ - according to the docs, "presence of this method indicates that the numeric object is an integer type". Also, it is supposed to be used for lossless conversion only, so using it in this case might be slightly abusive.
Would it be okay to have it be a normal method, like static_hash() (or another name if you have a better idea)?
I agree with the return type being an int, as it mimics what Python's hash() and it would be what I would naively expect.

On a side note, I went digging into why I kept the standard __hash__() method not idempotent, and here's why:
"By default, the hash() values of str and bytes objects are “salted” with an unpredictable random value. Although they remain constant within an individual Python process, they are not predictable between repeated invocations of Python.
This is intended to provide protection against a denial-of-service caused by carefully chosen inputs that exploit the worst case performance of a dict insertion, O(n2) complexity. See http://www.ocert.org/advisories/ocert-2011-003.html for details."

@Augustinio
Copy link
Collaborator Author

Got it, any approach is fine by me. A static_hash method is good.

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 a pull request may close this issue.

2 participants