mixin class for equality and equivalence

[DaniBodor]

This PR does the following:

• additions to docstring of slicing module (refactor: slicing mixin class #132 ) + add init file to mixins subpackage
• create a generic __eq__ function for alive data structures
• create a generic isequivalent function for alive data structures
• apply isequivalent throughout

[DaniBodor]

@psomhorst, in 30efe49, you say:

Before this commit, check_equivalence() had to be called as a class
method. Two variants a and b could be checked for equivalence by
calling Variant.check_equivalence(a, b) or
a.check_equivalence(a, b). The last command made little sense.

By removing the classmethod decorator Variant.check_equivalence(a, b)
still works. The second command is changed to a.check_equivalence(b),
which makes more sense.

However, I think that as a static method (the way you did it for Variant), the command a.check_equivalence(b) doesn't work, where as a standard instance method Variant.check_equivalence(a, b) doesn't work. Not sure if there's a way around either or whether I'm missing something.
We can also revert to class method, as the a.check_equivalence(a, b) command is kind of odd, but not problematic. The default way to call it would still be Variant.check_equivalence(a, b).

What's your opinion on this?

[DaniBodor]

Consider renaming check_equvalence to isequivalent, to be more in line with issubclass and isinstance, etc.
The behavior is slightly different, though, as this can optionally raise an error as an alternative output to returning a bool. So might not be a great idea to name it analogously to those builtins.

[psomhorst]

However, I think that as a static method (the way you did it for Variant), the command a.check_equivalence(b) doesn't work, where as a standard instance method Variant.check_equivalence(a, b) doesn't work. Not sure if there's a way around either or whether I'm missing something. We can also revert to class method, as the a.check_equivalence(a, b) command is kind of odd, but not problematic. The default way to call it would still be Variant.check_equivalence(a, b).

What's your opinion on this?

Then it should not be a static method but a normal method with self as first argument and other as second argument. You can call it as a.check_equivalence(b) (a becomes self, because the method is bound to the instance a) as Variant.check_equivalence(a, b) (a becomes self because it's not bound to an instance).

[psomhorst]

Consider renaming check_equvalence to isequivalent, to be more in line with issubclass and isinstance, etc. The behavior is slightly different, though, as this can optionally raise an error as an alternative output to returning a bool. So might not be a great idea to name it analogously to those builtins.

I agree that this is a better name. If raise_=False by default, the normal behaviour is to return a boolean. This is also in line with e.g. numpy, scipy and pandas.

[psomhorst]

Suggested change

	return Equivalence.__eq__(self, other)
	return self.variant_type == other.variant_type and super().__eq__(other)

[psomhorst]

This raises a KeyError if self contains keys that other does not. The second check in the dictionary above does check for this, but that does not result in an actually throws exception, just the value being False. So this code is being evaluated, even if set(self.keys()) != set(other.keys()).

Suggested change

	for key in self.keys():
	for key in set(self.keys()) & set(other.keys()):

[psomhorst]

Even more problematic is that self and other both can be objects that do not implement keys(). This method raises AttributeError if other is not a VariantCollection, just like the lines above.

[psomhorst]

This check leads to issues. This check runs regardless of the type of other (or even self, as it can be called as a class method). When testing this with other as something different from a VariantCollection, this raises an AttributeError because other.variant_type does not exist. These checks should ideally run after isequivalent() checks whether the type of other equals the type of self.

Elsewhere I proposed to make a separate method that performs these checks, which is called from isequivalent(). This method can be called after isequivalent() makes sure the type matches.

Alternatively, this check should first check whether other has a variant type.

Suggested change

	f"Variant types don't match: {self.variant_type}, {other.variant_type}": self.variant_type == other.variant_type,
	f"Variant types don't match: {self.variant_type}, {other.variant_type if hasattr(other, "variant_type") else ""}": hasattr(other, "variant_type") and self.variant_type == other.variant_type,

This becomes quite verbose quite quickly. Error messages do not properly convey the issue at hand.

[psomhorst]

This line has the same problem as above.

Suggested change

	f"VariantCollections do not contain the same variants: {self.keys()=}, {other.keys()=}": set(self.keys()) == set(other.keys()),
	f"VariantCollections do not contain the same variants: {self.keys()=}, {other.keys() if hasattr(other, 'keys') else ''=}": hasattr(self, "keys") and hasattr(other, "keys") and set(self.keys()) == set(other.keys()),

[DaniBodor]

@psomhorst , I've implemented a few of the changes you suggested, mainly on the core workings of the Equivalence class. The ones that are left now are kind of specific to some of the (more complicated) classes that will hold the data.

To me, the main objective of this PR is to create a structure/class that can be used generically for testing equality and equivalence. I think I re-coded all the separate checks that previously were written per subclass into this structure. Currently it is a bit hard for me to judge exactly how each class will work and what needs to be checked for each of them, and it might also still be a bit fluent.

I suggest that in this PR, we focus on the core of the Equivalence class, and then resolve the details for each daughter class as we are finalizing those classes. What do you think of that?
Alternatively, maybe we can have a call to fix the ones where you still have open comments, or discuss it during our next meeting, as right now I find it hard to see exactly what is going on in each.

[psomhorst]

I think this looks good! I have not looked at testing this extensively, but we'll run into problems when implementing classes using Equivalence anyway.

[psomhorst]

Suggested change

	raise f"{attr.capitalize()}s don't match: {s}, {o}"
	msg = f"{attr.capitalize()}s don't match: {s}, {o}"
	raise EquivalenceError(msg)

[DaniBodor]

I think this looks good! I have not looked at testing this extensively, but we'll run into problems when implementing classes using Equivalence anyway.

Since your review, I've rebased the entire branch to make the commits more self-sufficient, but functionally nothing has changed. I will merge as is.

As to your suggestion for the error message, I agree, but will do it in #143, as it is part of the ruff formatting, and will immediately tackle all existing error messages as well.