Update `<:AbstractRuleNode` equivalences

[THinnerichs]

HerbCore defines equivalence of anything <: AbstractRuleNode as follows:

➜  HerbCore git:(master) grep -rnw '.' -e 'Base'
...
./src/rulenode.jl:155:Base.:(==)(::RuleNode, ::AbstractHole) = false
./src/rulenode.jl:156:Base.:(==)(::AbstractHole, ::RuleNode) = false
./src/rulenode.jl:157:function Base.:(==)(A::RuleNode, B::RuleNode)
./src/rulenode.jl:163:Base.:(==)(A::AbstractHole, B::AbstractHole) = false
...

Source

HerbCore.jl/src/rulenode.jl

Lines 209 to 217 in 7bc381b

	Base.:(==)(::RuleNode, ::AbstractHole) = false
	Base.:(==)(::AbstractHole, ::RuleNode) = false
	function Base.:(==)(A::RuleNode, B::RuleNode)
	(A.ind == B.ind) &&
	length(A.children) == length(B.children) && #required because zip doesn't check lengths
	all(isequal(a, b) for (a, b) in zip(A.children, B.children))
	end
	# We do not know how the holes will be expanded yet, so we cannot assume equality even if the domains are equal.
	Base.:(==)(A::AbstractHole, B::AbstractHole) = false

Specifically, Holes are never equal.

This is overwritten in HerbConstraints for StateHole with proper definitions)

./src/solver/uniform_solver/state_hole.jl:88:function Base.:(==)(A::StateHole, B::StateHole)
./src/solver/uniform_solver/state_hole.jl:95:function Base.:(==)(A::RuleNode, B::StateHole)
./src/solver/uniform_solver/state_hole.jl:102:function Base.:(==)(A::StateHole, B::RuleNode)

Source

https://github.com/Herb-AI/HerbConstraints.jl/blob/26275e2a8bf99168ac45ab6a63644b4c62dca0fc/src/solver/uniform_solver/state_hole.jl#L88-L107

function Base.:(==)(A::StateHole, B::StateHole)
	isfilled(A) && isfilled(B) &&
    	(get_rule(A) == get_rule(B)) && 
		(length(A.children) == length(B.children)) &&
		all(isequal(a, b) for (a, b) in zip(A.children, B.children))
end


function Base.:(==)(A::RuleNode, B::StateHole)
	isfilled(B) &&
    	(get_rule(A) == get_rule(B)) && 
		(length(A.children) == length(B.children)) &&
		all(isequal(a, b) for (a, b) in zip(A.children, B.children))
end


function Base.:(==)(A::StateHole, B::RuleNode)
	isfilled(A) &&
    	(get_rule(A) == get_rule(B)) && 
		(length(A.children) == length(B.children)) &&
		all(isequal(a, b) for (a, b) in zip(A.children, B.children))
end

[ReubenJ]

Do we want to redefine Base.:(==) so that holes with the same domain are equivalent?

[THinnerichs]

That seems legit, but I honestly do not know whether that breaks anything.

[sebdumancic]

Do we want to redefine Base.:(==) so that holes with the same domain are equivalent?

I think this is the right thing to do this.

[ReubenJ]

That seems legit, but I honestly do not know whether that breaks anything.

It will surely break things in HerbConstraints, but I think that's a good thing because I've found some of the code somewhat difficult to read when relying on this equality check.

[ReubenJ]

For now, we define issame in #57.

[ReubenJ]

To keep the record of our discussion about this in one place:

I propose that we:

1. redefine == for holes so that it compares their domain field
2. update the hole1 == hole2 comparisons between holes in HerbConstraints to hole1 == hole2 && isfilled(hole1) (no need to check if both holes are filled if they have equivalent domains

[ReubenJ]

Check again: if we change the equivalence to == when domains are equal, do the tests in HerbConstraints fail?