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

Prevent other crates from implementing *Ext traits #34

Closed
jturner314 opened this issue Mar 18, 2019 · 3 comments · Fixed by #38
Closed

Prevent other crates from implementing *Ext traits #34

jturner314 opened this issue Mar 18, 2019 · 3 comments · Fixed by #38
Labels
Breaking changes Enhancement New feature or request

Comments

@jturner314
Copy link
Member

It would be nice to make it impossible for other crates to implement our *Ext traits, because then we could freely add new methods without breaking changes. (Adding the indexed_fold_skipnan method to MaybeNanExt in #33 is an example. If ndarray-stats was the only crate that could implement MaybeNanExt, then we could add indexed_fold_skipnan without that being a breaking change.)

ndarray accomplishes this for some of its traits (e.g. the Dimension trait) using a private marker type.

@jturner314 jturner314 added Enhancement New feature or request Breaking changes labels Mar 18, 2019
@LukeMathWalker
Copy link
Member

LukeMathWalker commented Mar 18, 2019

Would you suggest to do this for all our traits or just for some of them?
I do see the rationale, given that we are using traits just as extension mechanism for things that would actually belong to the implementation block of the ArrayBase struct.
At the same time, it would be nice (mostly for ndarray, not for ndarray-stats) to converge on a re-usable "array interface" that could be used to swap different implementations (e.g. sparse arrays, using things like sprs, vs ndarray's dense arrays), but it's probably premature given that a lot of upcoming languages feature could greatly impact the public API.

@jturner314
Copy link
Member Author

Would yousuggest to do this for all our traits or just for some of them?

All of the extension traits for ArrayBase, because it doesn't make sense for anyone else to implement them right now anyway. I could go either way on the other traits.

At the same time, it would be nice (mostly for ndarray, not for ndarray-stats) to converge on a re-usable "array interface" that could be used to swap different implementations (e.g. sparse arrays, using things like sprs, vs ndarray's dense arrays)

I agree, but a common array interface doesn't exist right now, and I'd rather focus on getting things working well for ndarray first. If a common array interface did exist, it would probably make sense to implement ndarray-stats's extension traits in terms of that interface. (I don't think ndarray-stats's extension traits make sense as part of a common array interface; they should be built on top of a common array interface.)

The way a lot of our traits are defined right now wouldn't make sense for types other than ArrayBase anyway. (Consider e.g. the S parameter to Sort1dExt. ndarray makes it impossible to implement Data in any other crates, so it wouldn't make sense for sprs's implementation of Sort1dExt to have an S: Data parameter.)

We can always remove the private marker type later without any breaking changes.

@LukeMathWalker
Copy link
Member

If a common array interface did exist, it would probably make sense to implement ndarray-stats's extension traits in terms of that interface. (I don't think ndarray-stats's extension traits make sense as part of a common array interface; they should be built on top of a common array interface.)

100% agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking changes Enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants