Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

relax type definition of middle #28

Merged
merged 2 commits into from
Oct 8, 2020
Merged

Conversation

tlnagy
Copy link
Contributor

@tlnagy tlnagy commented Mar 3, 2020

this adds support for computing the median of unitful types, fixes PainterQubits/Unitful.jl#202

@tlnagy
Copy link
Contributor Author

tlnagy commented Mar 3, 2020

Thoughts @nalimilan?

@ararslan
Copy link
Member

ararslan commented Mar 3, 2020

This could do with some tests. But that aside, I think Number might be a bit too permissive; for example, that allows Complex, for which middle isn't really well-defined.

@kleinschmidt
Copy link
Member

Why isn't that well-defined for Complex? Seems pretty straightforward to me but then again I am a fool when it comes to complex numbers so I could be missing something obvious...

@ararslan
Copy link
Member

ararslan commented Mar 3, 2020

middle is like the value between two numbers, i.e. the midpoint such that a <= x <= b but there isn't a well-defined order (no <=) for complex numbers. This is also why median isn't defined for collections of complex numbers.

@kleinschmidt
Copy link
Member

Could always add a mehod for Complex or whatever the most relevant abstract supertype is that throws a DomainError or something?

@tlnagy
Copy link
Contributor Author

tlnagy commented Mar 3, 2020

It would very nice to be able to compute medians of unitful types, but I understand your argument about Complex numbers. I would be happy with something like @kleinschmidt's suggestion.

@ararslan
Copy link
Member

ararslan commented Mar 3, 2020

Tangentially, I'm slightly surprised that unitful quantities subtype Number rather than Real; I'm having a hard time coming up with a concrete example of units for non-real numbers.

@tlnagy
Copy link
Contributor Author

tlnagy commented Mar 4, 2020

What is the best way of figuring out whether we can change the supertype of unitful quantities?

Seems risky to restrict the type from its current definition.

@jameslefevre
Copy link

Surely middle should be defined for complex numbers, or whenever x/2+y/2 is defined for that matter? Median will fail on complex numbers because PartialSort fails, which seems correct to me.

Tangentially, I'm slightly surprised that unitful quantities subtype Number rather than Real; I'm having a hard time coming up with a concrete example of units for non-real numbers.

I think impedance?

@nalimilan
Copy link
Member

@ararslan Given that allowing Number would help with Unitful and it's not incorrect per se for complex numbers, would you be OK with merging this PR? The fact that complex numbers shouldn't be allowed for median is a separate issue, which is handled anyway by the fact that sorting will fail.

@ararslan
Copy link
Member

Sure, sounds good to me.

@ararslan
Copy link
Member

Before doing so it would be good to update the docstring as Bogumił mentioned. Might also be nice to have a test for non-reals.

@nalimilan
Copy link
Member

OK, let's go with that then. @tlnagy Do you want to update the PR?

@tlnagy
Copy link
Contributor Author

tlnagy commented Oct 1, 2020

Thanks everyone for the constructive feedback, I've updated the PR. I think it's ready to merge!

This will close #43

@tlnagy
Copy link
Contributor Author

tlnagy commented Oct 8, 2020

bump

@ararslan ararslan merged commit 2439cc9 into JuliaStats:master Oct 8, 2020
@ararslan
Copy link
Member

ararslan commented Oct 8, 2020

Thanks @tlnagy! (And for the reminder!)

@nalimilan
Copy link
Member

To get this included in the upcoming 1.6.0-alpha release, would you make a PR similar to JuliaLang/julia#35869 to use the latest Statistics commit in Julia? Thanks!

@tlnagy
Copy link
Contributor Author

tlnagy commented Oct 8, 2020

Hi @nalimilan, I'm not sure how how to create a PR like that?

@nalimilan
Copy link
Member

The manual process is to copy the git SHA of the latest commit, put it in stdlib/Statistics.version, run make to download the tarballs, call md5sum and sha256sum on them and save the result to the files with the relevant names. I think there's a more automated way, but I don't remember it. :-/

@tlnagy
Copy link
Contributor Author

tlnagy commented Oct 8, 2020

And I should do that when building the latest julia master from source, right?

@tlnagy
Copy link
Contributor Author

tlnagy commented Oct 8, 2020

I submitted a PR (JuliaLang/julia#37950) against Base to bump Statistics. It would be great to have it in Julia 1.6!

@jebej
Copy link

jebej commented Oct 9, 2020

Tangentially, I'm slightly surprised that unitful quantities subtype Number rather than Real; I'm having a hard time coming up with a concrete example of units for non-real numbers.

frequencies: https://en.wikipedia.org/wiki/Laplace_transform

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

middle + Unitful does not work Cannot compute median
7 participants