Support unprotected mode for specials with FullRank schemas as well

[kleinschmidt]

This is a follow-up to #183 which extends @support_unprotect to provide support for arbitrary schema types, and uses that to generate support for specials (+, &, *, and ~) for both Schema and FullRank.

This is necessary to support cases like mixed models where we do things like (taking some liberties here with the actual method signature)

apply_schema(t::FunctionTerm{typeof(|)}, sch::FullRank, mod::Type{<:MixedModel})
    ...
    lhs, rhs = t.args
    lhs = apply_schema(lhs, sch, mod)
    ...
end

lhs may be 1, or 1 + x, or even log(x), so we can't blindly "lift" functionterms on the lhs (e.g., do something like lhs = lhs isa FunctionTerm ? lhs.f(lhs.args...) : lhs) because that would mangle log(x).

[nalimilan]

Would it make sense to introduce an AbstractSchema supertype instead?

[kleinschmidt]

Would it make sense to introduce an AbstractSchema supertype instead?

I actually first tried doing Union{Schema,FullRank}. Unfortunately, that leads to (you guessed it!) method ambiguities, because there are methods for ::FunctionTerm, ::Schema, ::Type and many more/less specific combinations. AbstractSchema would have the same problem I'm afraid :-/

[nalimilan]

Too bad. Then would it make sense to have @support_unprotect define methods for both Schema and FullRank by default?

[kleinschmidt]

Too bad. Then would it make sense to have @support_unprotect define methods for both Schema and FullRank by default?

yueah that's actually not a bad idea. it gets a bit tricky to write the macro that way though...maybe with a varargs it woudl be okay...let me play around a bit

[nalimilan]

Looks good. Can you add tests?

[kleinschmidt]

okay, another thing that's turned up in doing the MixedModels upgrade is that it would be very, very handy to have methods like unprotect(t::FunctionTerm{+}) = t.f(t.args...). This macro would be a good place to define that EXCEPT that you then would get "method overwritten" errors if invoked later (to add support for another schema type for instance), soooo I dunno.

[kleinschmidt]

(for the time being I think it's fine to vendor this in MixedModels as _unprotect or something; if it becomes necessary elsewhere we can upstream it here)

[palday]

(for the time being I think it's fine to vendor this in MixedModels as _unprotect or something; if it becomes necessary elsewhere we can upstream it here)

I'm okay with that --- MixedModels also has the whole MultiSchema stuff.

[kleinschmidt]

@nalimilan I've added tests (and fixed a very obvious bug that the tests caught, so thank you for that suggestion 😁 )

[nalimilan]

Why not define this simply as:

Suggested change

	function ft(f)
	return function(args...)
	return FunctionTerm(f, args, :($f($(args...))))
	end
	end
	ft(f, args...) = FunctionTerm(f, args, :($f($(args...))))

and call it as ft(op, a, b) instead of ft(op)(a, b)?

[kleinschmidt]

yeah that works too! I think the version here was inspired by the "lift" model of "passmissing" and friends. But for tests I'm kind of indifferent.

[kleinschmidt]

@nalimilan I updated teh tests based on your suggestion, read for re-review.