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

Implement approx traits for ArrayBase #581

Merged
merged 26 commits into from May 5, 2019

Conversation

jturner314
Copy link
Member

This implements the traits from the approx crate for ArrayBase. A couple of questions:

  • Should this be behind a feature flag?
  • Should the implementations broadcast like .all_close() does?
  • Should we remove .all_close() now?

One other note: The traits currently require the rhs to have type Self, which means comparisons don't work for differing storage types. We can fix this once brendanzab/approx#48 is merged.

@jturner314 jturner314 mentioned this pull request Jan 11, 2019
@LukeMathWalker
Copy link
Member

I think it should be behind a feature flag, as long as we don't use it in the library itself to provide other functionalities. There are similar nice-to-have trait implementations which are not core (i.e. quickcheck's trait) that we can provide as an additional feature without bloating the crate for people who don't need them.

@LukeMathWalker
Copy link
Member

Are the fate of all_close and broadcasting behaviour the only aspects to be finalised for this PR to be merged? @jturner314

@jturner314
Copy link
Member Author

I'd like to change the implementation to take advantage of the new Zip::all() method (#615).

I don't think we should implicitly broadcast, at least for the initial version, just to keep things simple and explicit. (The user can to perform a broadcast themselves with .broadcast() if needed.) We can always add support for broadcasting later if it turns out that would be useful.

I'm not sure what to do about .all_close(). I lean towards deprecating it and then removing it to minimize duplication. The approx crate is pretty small, so I wouldn't expect users to mind using it for this functionality.

@LukeMathWalker
Copy link
Member

I agree on all_close - I think we should strive to have a single way of doing things when we can.

Broadcasting is forward-compatible, so it's fine to go out with a more explicit approach first.

@LukeMathWalker
Copy link
Member

LukeMathWalker commented May 4, 2019

I have some free time, so I am working on a branch to finalize this (use Zip::all, deprecate all_close, allow comparing arrays with different ownership types). @jturner314

@jturner314
Copy link
Member Author

I accidentally broke this (the approx impls must match the PartialEq impls, but PartialEq is currently only implemented for arrays of the same element type). I've generalized the PartialEq impl for differing element types in #588. Let's merge that, and that should fix this PR.

@LukeMathWalker
Copy link
Member

It seems to be working now 👍

@jturner314
Copy link
Member Author

I just noticed that there are a few things to fix, which I'm working on now. (Tests annotated with #[cfg(features = "approx")] are ignored even with the approx feature enabled. The annotation should be #[cfg(feature = "approx")] instead.)

@LukeMathWalker
Copy link
Member

My bad, silent failure 😅

@jturner314
Copy link
Member Author

No worries, that's a really easy mistake to make. I'm surprised the compiler didn't give a warning about it.

This PR looks ready to merge (assuming CI passes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants