Support Subtraction of expressions - Githubissues

TabletopAssistant / DiceKit

Swift framework for expressing and evaluating dice notation (e.g., d20, 4d6+4, 3d8×10+2), which is commonly used in tabletop role-playing games.

Apache License 2.0

14 stars 1 forks source link

Support Subtraction of expressions #80

Closed JonathanHoffman closed 8 years ago

JonathanHoffman commented 9 years ago

TODO:

[x] Add dictionary support for filter method
[x] Add method to remove zeroes from a frequency distribution
[x] Add subtract method for frequency distributions
[x] Add divide method for frequency distributions
[x] Add method to deconvolute a probability mass function
[x] Add subtraction type
[x] Add examples to playground, update readme
[x] Expand dictionary filter method

Notes:

~~Deltas: Should I pass the delta we want to the removeZeroes function, or just use the default set in ProbabilityMassConfig? Or just set delta to the value where ever it is used?~~
So far I have only implemented the filter function that I need. Should I add more?
I spent a lot of time designing divide, but I think more testing is needed. I might implement a SwiftCheck for it.
Divide can also be implemented and optimized using FFT. I think our implementation could have a O(n log n) upper bound on complexity (currently O(n²) I think).

Closes #63

brentleyjones commented 9 years ago

So far I have only implemented the filter function that I need. Should I add more?

Yes. Add the one for keys and the overall one, so 2 more.

brentleyjones commented 9 years ago

Deltas: Should I pass the delta we want to the removeZeroes function, or just use the default set in ProbabilityMassConfig? Or just set delta to the value where ever it is used?

Leave as is, passing in a value.

brentleyjones commented 9 years ago

In a new commit: Now that we have filterZeroFrequencies, let's do like ProbabilityMass does, and expose an internal initializer that has a flag to tell it to filter. That way if someone is evil and passing in [5: 0.0] we can make it [:] instead. Use a delta of 0.0 for that one. For nearly all local calls to our initializer, like after every operation, we would pass false, just like ProbabilityMass does. In subtract though we would pass true.

This would also fix normalizeFrequencies in the zero case, since it won't have any elements to map over and get a division by zero error. This may have bigger implications, but I think it's worth fixing those than to allow people to pass in invalid frequencies.

brentleyjones commented 9 years ago

I know it's not part of this, but as part of the PR can you update README Usage section on expressions? Maybe use - instead of + and change the comments to be more descriptive.

JonathanHoffman commented 9 years ago

I've updated the title and description. This is ready for review. :mag:

brentleyjones commented 9 years ago

Just the one comment left. Really nice work @JonathanHoffman!

brentleyjones commented 9 years ago

After the CHANGELOG.md change, go ahead and merge. :+1:

brentleyjones commented 8 years ago

The build failing was my issue. I've just fixed it. After you rebase it should pass just fine.