Make PValue and TestStat behave like Reals

[palday]

• Add comparison operator methods for PValue and TestStat
• Add tests for show(::TestStat)
• Make PValue a subtype of Real

[nalimilan]

Suggested change

	for op in [Symbol("=="), :≈, :<, :≤, :>, :≥]
	for op in [:(==), :≈, :<, :≤, :>, :≥]

Maybe also define isequal and isless?

Actually I wonder whether it would be simpler to define promote_type(::Type{PValue{S}}, y::T) where {S, T<:Real} = promote_type(S, T), making S a type parameter with v::S. Then I think we'll get all operations on numbers defined for free.

[palday]

You get isequal and isless for free with the symbolic forms (and I use isequal and isless) in the tests.

I was also thinking about whether promote_type might be the easier route. I'll try that later. :)

[nalimilan]

Ah yes so isless falls back to <, but it's not correct for NaN, right?

[palday]

Uh.... good question

[palday]

Oh wait, I know why I didn't do promote_type: that would be technically breaking, both for PValue and TestStat. We're pre 1.0 so another release isn't a big deal. But that might be something to get one more opinion on (e.g. @ararslan).

(I also want PValue and TestStat to have a consistent interface between themselves since they're both essentially just pretty printers for a semantically annotated Real.)

[nalimilan]

You mean it would be breaking if you do something like [pval, 1.0]? Yes that would be slightly breaking so that would probably warrant tagging a minor release.

[palday]

Yeah, and adding a type parameter to a struct also breaks (de)serialization.

[nalimilan]

Maybe let's keep your current approach then, and only use promote_type in another PR that would be merged only once we have other breaking changes to tag.

[palday]

As soon as this one (and CoefTable show method) lands, I'll open that PR 😄

[nalimilan]

Damn, this is really tricky. Since we now define isequal, it would be good to check that hash is also consistent. Otherwise putting PValue objects in a dict will give incorrect results.

AFAICT that's the case thanks to the hash(x::Real, h::UInt) fallback, but the only way to be sure is to test it. Defining hash(x::PValue, h::UInt) = hash(x.val, h) would probably be more efficient.

Out of curiosity, in what use case did you feel the need to this? :-) I realize that PValue and TestValue were originally essentially simple wrappers to customize printing internally which were not exposed to users, but we're now making them full-fledged types.

[palday]

Damn, this is really tricky. Since we now define isequal, it would be good to check that hash is also consistent. Otherwise putting PValue objects in a dict will give incorrect results.

AFAICT that's the case thanks to the hash(x::Real, h::UInt) fallback, but the only way to be sure is to test it. Defining hash(x::PValue, h::UInt) = hash(x.val, h) would probably be more efficient.

Hahahahah, I'll add this as well. And as soon as we get this and the show method tagged for a patch release, I'll open a PR for the promote-rule which will be breaking but just avoid so much of this hassle. 😄

Out of curiosity, in what use case did you feel the need to this? :-) I realize that PValue and TestValue were originally essentially simple wrappers to customize printing internally which were not exposed to users, but we're now making them full-fledged types.

We had something at the dayjob where there was some conditional behavior based on a p-value threshold (I know, I don't like it either, but so much of the applied stats world still does that). In one instance, the p-values were already wrapped for pretty printing (and expensive to re-compute) and we had to add the filter at the very end. When doing that, I noticed that TestStat already subtyped Real, so I thought it made sense to just say "okay, these are Reals with pretty printing (and bounds checking for PValue), so let's make them behave that way". I guess I'm not the only person who will say "give me all the rows in this CoefTable where p < 0.05", so it will be nice to have comparison operators Just Work™.

[nalimilan]

Hahahahah, I'll add this as well. And as soon as we get this and the show method tagged for a patch release, I'll open a PR for the promote-rule which will be breaking but just avoid so much of this hassle. smile

Thanks!

We had something at the dayjob where there was some conditional behavior based on a p-value threshold (I know, I don't like it either, but so much of the applied stats world still does that). In one instance, the p-values were already wrapped for pretty printing (and expensive to re-compute) and we had to add the filter at the very end. When doing that, I noticed that TestStat already subtyped Real, so I thought it made sense to just say "okay, these are Reals with pretty printing (and bounds checking for PValue), so let's make them behave that way". I guess I'm not the only person who will say "give me all the rows in this CoefTable where p < 0.05", so it will be nice to have comparison operators Just Work™.

OK. But do you mean you get PValue objects when extracting values from CoefTable objects? I thought it was used only internally for printing, but never returned to users.

[palday]

Uh.... I don't know. (We're not using CoefTable at that point in the actual example I was looking at.)

[ararslan]

...come to think of it, how did we end up with PValues? 🤔

[palday]

A custom show method working on a column table where we assumed that the element-level pretty printing was handled via show for that eltype.

And when this comparison wasn't implemented, everything broke.