Skip to content

Fix overflow for Midpoint #28

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

Merged

Conversation

LukeMathWalker
Copy link
Member

This provides a fix for #27, I have also included a test to reproduce the issue so that we can be aware of regressions.
Unfortunately, it's a breaking change PR because I have had to add Sub to the trait bound. Should we just use NumOps for all Interpolate implementations @jturner314?

@LukeMathWalker LukeMathWalker added Bug Something isn't working Breaking changes labels Feb 24, 2019
Copy link
Member

@jturner314 jturner314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR looks good to me.

Should we just use NumOps for all Interpolate implementations @jturner314?

I don't think so, because e.g. Lower and Higher don't need to do any arithmetic. The reason why we have an Interpolate trait anyway is to minimize the trait bounds on the element types. (Otherwise, if we require the same trait bounds for all element types, we might as well just replace all the the types that implement Interpolate with a single enum.)

@LukeMathWalker
Copy link
Member Author

LukeMathWalker commented Feb 25, 2019

Sorry, my wording was not precise. I meant using NumOps as trait bounds for all those requiring mathematical operations (mainly Linear and Midpoint). It's very easy to be minimal (Add but not Sub) but this leads to needless breaking changes in the API (such as this one) which could probably be avoided by saying "you can use this if we can do arithmetics with your type".

@jturner314
Copy link
Member

Oh, I understand now. Sure, requiring NumOps for Linear and Midpoint seems reasonable and would help avoid breaking changes in the future.

By the way, thanks for taking care of this issue!

@LukeMathWalker
Copy link
Member Author

By the way, thanks for taking care of this issue!

I am actually quite thrilled that this crate has its first legit user (and bug report, as it often goes) 🚀

@LukeMathWalker
Copy link
Member Author

I have added the NumOps trait constraint - I'll squash and merge.

@LukeMathWalker LukeMathWalker merged commit 4e01b15 into rust-ndarray:master Feb 26, 2019
@LukeMathWalker LukeMathWalker deleted the fix-overflow-for-midpoint branch February 26, 2019 10:10
@jturner314 jturner314 mentioned this pull request Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking changes Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants