Hashable and Immutable ParamSpec

[astafan8]

Introduction of __eq__ for ParamSpec introduced implicit __hash__ = None definition into the class (according to Python 3 rules) which cause some rare code that was making sets of ParamSpecs break.

This PR adds __hash__ method to ParamSpec that allows to create use ParamSpecs as other hashable objects, for example, making sets of them.

Moreover, ParamSpec is made immutable. add_depends_on and add_inferred_from methods have been removed.

Also, depends_on_ and inferred_from_ properties (with _ at the end) have been added which return a list of strings as opposed to a string with comma-separated names.

• change pytopo/sweep to not use add_depends_on and add_inferred_from methods

[WilliamHPNielsen]

What is dangerous, however, is that ParamSpec is still a mutable object via the add_depends_on and add_inferred_from methods. In some earlier PR it was decided to get rid of those soon.

I think it could be made part of this PR to remove those two methods.

[codecov]

Codecov Report

Merging #1367 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
- Coverage   73.15%   73.11%   -0.04%     
==========================================
  Files          79       79              
  Lines        9156     9144      -12     
==========================================
- Hits         6698     6686      -12     
  Misses       2458     2458

[codecov]

Codecov Report

Merging #1367 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
+ Coverage   73.47%   73.51%   +0.03%     
==========================================
  Files          92       92              
  Lines       10239    10253      +14     
==========================================
+ Hits         7523     7537      +14     
  Misses       2716     2716

[sohailc]

You seem to know what you are doing, but can you explain why XOR?

[astafan8]

it pops up within the first 10 google search items when you look for "creating hash in python" or similar :) and the comments there seem convincing.

[sohailc]

What is dangerous, however, is that ParamSpec is still a mutable object via the add_depends_on and add_inferred_from methods. In some earlier PR it was decided to get rid of those soon.

I think it could be made part of this PR to remove those two methods.

The add_depends_on and add_inferred_from are used in pysweep. See: https://github.com/kouwenhovenlab/pytopo/blob/master/pytopo/sweep/param_table.py

line 97. If you want a working pysweep @astafan8, do not remove those!

[astafan8]

@QCoDeS/core ready for review

[jenshnielsen]

Looks good but there seems to be some intermittent issue with the hypothesis generation

    @given(paramspecs=hst.lists(valid_paramspec_kwargs, min_size=6, max_size=6),
>          add_to_1_inf=hst.booleans(),
           add_to_1_dep=hst.booleans(),
           add_to_2_inf=hst.booleans(),
           add_to_2_dep=hst.booleans(),
           )
    def test_hash_with_deferred_and_inferred_as_paramspecs(
            paramspecs, add_to_1_inf, add_to_1_dep, add_to_2_inf, add_to_2_dep):
        """
        Test that hashing works if 'inferred_from' and/or 'depend_on' contain
        actual ParamSpec instances and not just strings.
E       hypothesis.errors.FailedHealthCheck: Data generation is extremely slow: Only produced 9 valid examples in 1.07 seconds (0 invalid ones and 0 exceeded maximum size). Try decreasing size of the data you're generating (with e.g.max_size or max_leaves parameters).
E       See https://hypothesis.readthedocs.io/en/latest/healthchecks.html for more information about this. If you want to disable just this health check, add HealthCheck.too_slow to the suppress_health_check settings for this test.

[jenshnielsen]

Assuming this works well with the Sweep this is ready to land

[astafan8]

cool. may we give it an extra week? (the sweep has been updated some days ago, users have been informed, but not sure that everyone was updated) unless, this blocks the "interdependencies refactoring PR" very prominently.

[jenshnielsen]

Sounds like a good idea, merging this on a Friday is probably not the best idea

[jenshnielsen]

@astafan8 Should we merge this?