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

Fix reductions in Statistics #183

Merged
merged 2 commits into from
Jun 29, 2020
Merged

Fix reductions in Statistics #183

merged 2 commits into from
Jun 29, 2020

Conversation

timholy
Copy link
Member

@timholy timholy commented Jun 18, 2020

Formerly we were choosing an inconsistent eltype for reductions performed by Statistics. Since that's a stdlib that comes for "free," it seems we should support it on similar footing to reduce etc.

@codecov
Copy link

codecov bot commented Jun 18, 2020

Codecov Report

Merging #183 into master will decrease coverage by 0.20%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #183      +/-   ##
==========================================
- Coverage   87.84%   87.64%   -0.21%     
==========================================
  Files           5        5              
  Lines         436      437       +1     
==========================================
  Hits          383      383              
- Misses         53       54       +1     
Impacted Files Coverage Δ
src/FixedPointNumbers.jl 80.64% <0.00%> (-0.88%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0872417...c9ca80b. Read the comment docs.

src/FixedPointNumbers.jl Outdated Show resolved Hide resolved
@timholy
Copy link
Member Author

timholy commented Jun 25, 2020

Any objections or should I merge?

@timholy timholy merged commit 2f2a34a into master Jun 29, 2020
@timholy timholy deleted the teh/stats branch June 29, 2020 13:37
@kimikage
Copy link
Collaborator

kimikage commented Jul 5, 2020

@timholy, are these #183, #180 and #177 in v0.8.1 v0.8.2? Or in the v0.9 series?

As you mentioned, #177 is a somewhat dangerous change, but it shouldn't affect the "normal" functionality of downstream packages if there is no type piracy.

#186 is not a breaking change, and is not urgent. #187 is somewhat dangerous because it prohibits copysign/flipsign for Normed.

@timholy
Copy link
Member Author

timholy commented Jul 6, 2020

#180 is in 0.8.1. None of the rest have been released yet. I think everything merged can be in the 0.8.x series, though, see #177 (comment).

@kimikage
Copy link
Collaborator

kimikage commented Jul 6, 2020

Ah, I've already used v0.8.1, but I forgot it. 😅
My interest is when to release v0.9. Originally in v0.9, I was planning to improve arithmetic.

@timholy
Copy link
Member Author

timholy commented Jul 6, 2020

I'd say we could make a 0.8.2 release and then add any breaking changes.

Where do we stand on overflow? Without a clear consensus I'm reluctant to pick #143 up again. There's a part of me that thinks we should make all arithmetic immediately promote to floattype and just declare that these are storage-only numbers.

@kimikage
Copy link
Collaborator

kimikage commented Jul 6, 2020

I will not drastically change the promotion rules unless there are concrete proposals. In other words, there has been no debate in the last six months, so I want to improve the arithmetic.

My first goal is to complete the set of checked_*, wrapping_*, and saturating_*.(cf. #152 (comment))

Edit:
I submitted a draft PR #190.

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.

2 participants