Fix hashing of aggregate classes, rename reverse_order to sort_ascending

[bdice]

Description

Fixes a few minor issues in #348 that I caught after it was merged.

Motivation and Context

I will add comments in the PR discussing the motivation for my changes.

Types of Changes

• Documentation update
• Bug fix
• New feature
• Breaking change¹

¹The change breaks (or has the potential to break) existing functionality.

Checklist:

• I am familiar with the Contributing Guidelines.
• I agree with the terms of the Contributor Agreement.
• My name is on the list of contributors.
• My code follows the code style guideline of this project.
• The changes introduced by this pull request are covered by existing or newly introduced tests.

If necessary:

• I have updated the API documentation as part of the package doc-strings.
• I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
• I have updated the changelog.

[bdice]

We should call this sort_ascending with a default value of True so it matches Pandas' sort_values(by=..., ascending=True, ...). I find "ascending" unambiguous, while "reverse order" is less clear. https://pandas.pydata.org/pandas-docs/stable/reference/api/pandas.DataFrame.sort_values.html

[kidrahahjo]

We sort in ascending order by default.
Hence we'll always have to use the not keyword.
While this will definitely work, my preference here would be using sort_descending

[bdice]

I believe I only had to invert its value twice. Once in the call to sorted and also once in the check for “default values” where I replaced not reverse_order with sort_ascending. So it was an equal number of “nots” in each direction, which seems no more complex to me.

[vyasr]

I don't have a strong opinion either way, but this usage is distinct enough from anything you would do with pandas that I don't think there's any reason for confusion. I'm assuming the performance difference is negligible, but if that's not the case and the default sorting is the opposite of what pandas does I think choosing performance over exact API consistency with pandas is unnecessary here. I'm fine with whatever decision is made, though.

[kidrahahjo]

@bdice @vyasr Then we can stick with how @bdice implemented this.

[b-butler]

I don't feel this needs to conform to pandas, but I do think the naming is clearer with sort_ascending or sort_descending.

[bdice]

We should hash the bytecode before returning, since this is purely used as a unique identifier. We also shouldn't catch Exception! We know this should raise an AttributeError if it can't access the attribute.

[vyasr]

See my comment above (where this function is used).

[bdice]

We're severely over-complicating things here. Just take the hash of a tuple of hashable objects that uniquely define this instance. There's no need to do all the string operations and sha1 digests.

The only required property is that objects which compare equal have the same hash value; it is advised to mix together the hash values of the components of the object that also play a part in comparison of objects by packing them into a tuple and hashing the tuple.
https://docs.python.org/3/reference/datamodel.html#object.__hash__

[vyasr]

Does the _get_unique_function_id even need to exist? @kidrahahjo why were you hashing just the bytecode of the function (which is what _get_unique_function_id returns for functions) rather than just hashing the function directly? I honestly don't remember thinking through whether it was necessary, I think I just assumed there was a reason you bothered to define that rather than just hashing the function directly and moved on. Any object with a __code__ attribute should already be hashable though, right?

[kidrahahjo]

@vyasr Since aggregator_function is defined internally hence the hash would depend on it's location.
I believe this was the main reason for not creating a hash of a method directly.

Edit: I'll document this and add a commit now.

[bdice]

This comment is just for others' reference. In my understanding, _get_unique_function_id is useful for the same reason we use obj.__code__.co_code for the "tags" on _condition objects in flow. It allows us to know that two function objects are the same if their bytecode is equivalent (even if they're not actually located in the same object). Hashing the bytecode (when possible) is a helpful way to uniquely identify the aggregator/select function.

Demonstration of how hashes of functions are not equal (even though the functions are equivalent), but hashing the bytecode shows their equivalence:

>>> def foo():
...   print('asdf')
...
>>> def bar():
...   print('asdf')
...
>>>
>>> foo is bar
False
>>> hash(foo)
8793327702568
>>> hash(bar)
8793327702577
>>> foo.__code__.co_code
b't\x00d\x01\x83\x01\x01\x00d\x00S\x00'
>>> bar.__code__.co_code
b't\x00d\x01\x83\x01\x01\x00d\x00S\x00'
>>> assert hash(foo.__code__.co_code) == hash(bar.__code__.co_code)

[vyasr]

@kidrahahjo @bdice yes, that is true, but we're not using hashing to check equality or depending on being able to check equality at all any more since, as per our discussions, checking co_code is insufficient: for instance:

>>> def f(x):
...     return x**2
...
>>> def g(x):
...     return x**3
...
>>> f.__code__.co_code
b'|\x00d\x01\x13\x00S\x00'
>>> g.__code__.co_code
b'|\x00d\x01\x13\x00S\x00'

Although I pointed out some ways to get around the particular problem above (as well other even more subtle ways in which this comparison can fail), the point is that this id isn't really unique, right? So do we gain anything by defining a function whose name is not accurate and whose primary purpose is no longer valid? If Aggregate.__eq__ only returns True if (roughly) a is b, then wouldn't it make sense for our hashes to also be distinct?

[vyasr]

Right. Actually the situation even gets worse than that, I discussed some cases where comparing __code__.co_consts is also not sufficient (and possible ways to get around those edge cases as well). However, overall I think there are enough such possibilities that it's safest to avoid the problem altogether and just require strict object equality, which is also what we changed __eq__ to do. @kidrahahjo could you make the relevant change here (on a separate branch/PR at this point)? Basically just remove the usage of this helper function and directly hash the object unless this will cause any other problems. @b-butler feel free to comment if you think I'm missing something here, but I think this is directly in line with my suggestion for __eq__.

[b-butler]

@bdice @vyasr The __hash__ method does not have to distinguish all elements that are not equivalent it just requires that a == b -> hash(a) == hash(b). We can do this for now, but I think eventually having a hash method that can distinguish between commonly used aggregates would be useful. https://docs.python.org/3/reference/datamodel.html#object.__hash__

I don't think there is any safety concern given since if a is b then a == b and hash(a) == hash(b) in our model. __hash__ is not necessarily meant in Python to provide a unique id. It is more for hashable collections like dict and set. We can safely give objects the same hash when they are not equal as long as our __eq__ gives no false positives which the is operator ensures.

Edit: Fixed link

[vyasr]

I agree, in general hashes don't need to be equivalent and we only depend on __eq__. I just figured that for now we should keep the two roughly in sync.

signac at present basically assumes no hash collisions... but that's another story.

[bdice]

To be clear, hash collisions in signac would be caught and raise errors. I believe we have tests for this behavior. (Just didn’t want readers of the above comment to fear unexpected behavior.)

[vyasr]

Yes, hash collisions where two statepoints refer to the same job ID will raise an error. Nothing will break.

AFAIK hash collisions will raise the same DestinationExistsError as is raised when actually creating two identical jobs though. I'm not aware of any logic that explicitly checks for the reason and indicates to the user that there's a hash collision, so the error will look the same as if the user did something like

j1 = project.open_job(dict(a=1)).init()
j2 = project.open_job(dict(a=1, b=2)).init()
del j2.sp.b  # This will raise a DestinationExistsError because now j2 and j1 have the same statepoint.

In the event that a user somehow managed to trigger a collision (which is obviously highly unlikely, and not something that I think we need to deal with), the resulting error message would prevent any corruption of the data space, it would just be very confusing. That's all I was referring to, nothing needs to be done there.

[bdice]

Initialize empty lists/dicts with object literals. It's faster and more Pythonic.

[bdice]

This probably won't be parsed correctly by Sphinx. :py:class: will create a cross-link to the corresponding class, but we just want a code snippet.

[bdice]

This was not a safe hash function. The project id is just a name, and isn't actually a unique identifier (two projects in different locations can have the same name, and thus the same id). For the Project's __eq__ method, we use repr(self) == repr(other), because the repr includes a unique identifier: the project's path on disk. I use the same approach here.

[bdice]

The separate function was a little bit overkill -- it's only used once.

[vyasr]

My original intent intent in requesting a separate function was to also include the logic for blob construction (which was being done in many places in this file), but as you've pointed out you can always just hash a tuple of things rather than build up that object which is better :)

[kidrahahjo]

Thank you @bdice for reviewing #348 and fixing the issues through this PR.
I added a few commits which address your comments related to using fully qualified import statements.
Apart from this, it looks good to me. Thanks once, again.

[b-butler]

Thanks for the changes @bdice. They look good.

[b-butler]

I don't feel this needs to conform to pandas, but I do think the naming is clearer with sort_ascending or sort_descending.