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

Solves the inconsistency #10729 and add tests #10736

Merged
merged 1 commit into from
Apr 8, 2015

Conversation

lbenet
Copy link
Contributor

@lbenet lbenet commented Apr 3, 2015

No description provided.

@timholy
Copy link
Member

timholy commented Apr 3, 2015

Thanks for getting to this so quickly!

I guess my only (minor) question is whether we need to worry about the performance. I don't have much insight into whether the increasing complexity of these functions will affect their performance in practice. Have you done any benchmarking compared to the version before #9416?

Overall, though, correctness seems more important than speed.

@lbenet
Copy link
Contributor Author

lbenet commented Apr 3, 2015

No, I haven't done any benchmarks. I agree that this is not an elegant solution at all, and may indeed have an impact in speed. I simply put it there to avoid further problems, e.g. Winston.

Yet, after reading #7866, I realized that according to the standard, min and max should not care about the sign of zero. This is inconsistent is present in v0.3, in the sense that min(0.0,-0.0) and min(-0.0,0.0) differ in the sign bit.

@StefanKarpinski
Copy link
Member

Ugh. NaNs are the worst.

@StefanKarpinski
Copy link
Member

The tests look good and correctness is more important. I'll see if I can figure out a way to make this faster.

@StefanKarpinski
Copy link
Member

I guess we should merge this and then work on making it faster.

StefanKarpinski added a commit that referenced this pull request Apr 8, 2015
Solves the inconsistency #10729 and add tests
@StefanKarpinski StefanKarpinski merged commit 11ca993 into JuliaLang:master Apr 8, 2015
@lbenet lbenet deleted the lbenet/minmax branch April 9, 2015 21:33
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.

3 participants