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

Allow obtaining the mean of a distribution #285

Merged
merged 2 commits into from
Mar 17, 2024

Conversation

haykam821
Copy link
Contributor

Fixes #284

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 91.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 82.98%. Comparing base (5e04cb9) to head (4580c3a).
Report is 7 commits behind head on main.

Files Patch % Lines
core/src/num/dist.rs 86.66% 2 Missing ⚠️
core/src/error.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
+ Coverage   82.89%   82.98%   +0.09%     
==========================================
  Files          52       52              
  Lines       14947    15000      +53     
==========================================
+ Hits        12390    12448      +58     
+ Misses       2557     2552       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haykam821
Copy link
Contributor Author

I'm not sure how the 'there must be at least one part in a dist' error can be produced; I assume it's a failsafe prevented by parsing (e.g. d0 is invalid). I assume I can ignore the missing coverage as a result.

@bgkillas
Copy link

probably too annoying to implement for the little performance difference in most scenarios(aspecially because of how poor the performance of dice is to begin with) but to get the mean for dice it can just be (dice.sum()+dice.len())/2 via some simple logic about how the distribution is in the center always, and wouldn't mode/median be the same as mean? unless there are non dice distributions then it makes sense

@haykam821
Copy link
Contributor Author

haykam821 commented Mar 15, 2024

I realize that I implemented the mean calculation incorrectly; the current formula should work for all currently supported distributions, but a skewed distribution will have an incorrect mean due to the formula not accounting for probabilities.

The correct calculation is more like the following, with no division at the end:

result = Exact::new(k, true)
	.mul(&Exact::new(Complex::from(Real::from(v)), true), int)?
	.add(result, int)?;

The big rational to real to complex to exact conversion is a bit verbose, so if there's a more direct way I can do this, I can use that method instead.

@printfn
Copy link
Owner

printfn commented Mar 16, 2024

Thanks for the PR! It is actually already possible to generate skewed distributions with fend, for example:

> d6 / d2
0.5:  8.33%  ###############
  1: 16.67%  ##############################
1.5:  8.33%  ###############
  2: 16.67%  ##############################
2.5:  8.33%  ###############
  3: 16.67%  ##############################
  4:  8.33%  ###############
  5:  8.33%  ###############
  6:  8.33%  ###############
> mean(d6 / d2)
approx. 2.8333333333
> 0.5*1/12 + 1*2/12 + 1.5*1/12 + 2*2/12 + 2.5*1/12 + 3*2/12 + 4/12 + 5/12 + 6/12
2.625

Would you be able to fix this bug and add a test case for it?

I'm not sure how the 'there must be at least one part in a dist' error can be produced; I assume it's a failsafe prevented by parsing (e.g. d0 is invalid). I assume I can ignore the missing coverage as a result.

The code coverage checks are just to remind people to write the occasional test. There's no need to worry about small areas of missed coverage like that.

The big rational to real to complex to exact conversion is a bit verbose, so if there's a more direct way I can do this, I can use that method instead.

You're right, there's no shorter way to do that conversion atm unfortunately.

@haykam821
Copy link
Contributor Author

Thank you! I had a feeling there was something I missed that would allow for skewed distributions. In that case, I've committed the fix along with test cases.

@printfn printfn merged commit 3238994 into printfn:main Mar 17, 2024
6 checks passed
@haykam821 haykam821 deleted the distribution-mean branch March 17, 2024 18:50
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.

Support for obtaining the mean of a distribution
3 participants