-
Notifications
You must be signed in to change notification settings - Fork 297
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
Make mapv_into_any() work for ArcArray, resolves #1280 #1283
base: master
Are you sure you want to change the base?
Conversation
src/impl_methods.rs
Outdated
@@ -2606,14 +2618,14 @@ where | |||
}; | |||
// Delegate to mapv_into() using the wrapped closure. | |||
// Convert output to a uniquely owned array of type Array<A, D>. | |||
let output = self.mapv_into(f).into_owned(); | |||
let output = self.mapv_into(f); | |||
// Change the return type from Array<A, D> to Array<B, D>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation does not seem to match the types in the comments after this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, I forgot to update the comments. We are no longer using a concrete Array
as the return type, so the into_owned()
is no longer needed. Fixing the comments with a force push.
src/impl_methods.rs
Outdated
// Again, safe because A and B are the same type. | ||
unsafe { unlimited_transmute::<Array<A, D>, Array<B, D>>(output) } | ||
unsafe { unlimited_transmute::<ArrayBase<S, D>, ArrayBase<T, D>>(output) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like S and T can be completely different types, can we show how this would be safe, I don't see how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, you are right! 😳 The array representation of S
might not be the same as the array representation of T
, so even thought the element types A
and B
are the same, the types S
and T
might not be the same.
Fortunately, there is no type-related problem in Rust that cannot be solved with more traits! Using the Plug
trait from here we can introspect on the type of the data representation (e.g. OwnedRepr
vs OwnedArcRepr
) and perform the conversion soundly. I would appreciate feedback on where to put the Plug
trait and whether it is acceptable to expose such a trait through the API at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not on crates.io so it cannot be a dependency. I will decline to look into this unfortunately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the idea is add a plug trait as proposed here in the current revision already, not add a dependency on some crate. The linked repository has an explanatory character as well which is why it isn't a published crate if I understand this correctly.
So there is very little code involved to establish the relevant mappings on a type level, but the resulting API is definitely on the complex end of the scale w.r.t. its type bounds and hence probably not particularly simple to explain.
e8a73e6
to
2ac3c9f
Compare
src/data_traits.rs
Outdated
/// Plug the data element type of one owned array representation into another. | ||
/// | ||
/// For example, `<OwnedRepr<f64> as Plug<f32>>::Type` has type `OwnedRepr<f32>`. | ||
pub trait Plug<A> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the way this is being used, I think it would have to be an unsafe trait because implementing it for representation with incompatible memory layouts, the transmutes based on this would be unsound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Personally, since this is not a general exposition, I would also suggest giving the trait a named geared towards its usage here, something like SameRepr<A>
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This trait looks similar to the existing RawDataSubst.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamreichold, could you expound on this a little more? I think you are talking about marking the Plug
trait as unsafe to implement because someone could inadvertently use the wrong outer type in the implementation. For example:
impl <A, B> Plug<A> for OwnedRepr<B> {
type Type = OwnedArcRepr<A>; // Wrong! Should be OwnedRepr
}
Depending on the mistake, the conversion on line 2627 might still compile and the transmutation on line 2632 would be unsound.
Maybe instead of making Plug
unsafe I can add a runtime assertion that the plugged type is equal to T
. This assertion would fail if Plug
is implemented incorrectly as above.
assert!(core::any::TypeId::of::<<T as Plug<A>>::Type>() == core::any::TypeId::of::<T>());
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bluss Yes, and your encyclopedic knowledge of the trait system in ndarray is impressive! 💪
Perhaps Plug
should be renamed DataOwnedSubst
? I could also experiment with a blanket impl...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are talking about marking the Plug trait as unsafe to implement because someone could inadvertently use the wrong outer type in the implementation.
Exactly.
Maybe instead of making Plug unsafe I can add a runtime assertion that the plugged type is equal to T. This assertion would fail if Plug is implemented incorrectly as above.
I think you could actually use RawDataSubst
instead of adding a new trait? At least it does seem to provide the relevant implementations for OwnedRepr
and OwnedArcRepr
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I think everything has been addressed in the latest push.
T: DataOwned<Elem = B> + RawDataSubst<A> + 'static, // lets us introspect on the types of array representations containing different data elements | ||
<T as RawDataSubst<A>>::Output: RawData, // required by mapv_into() | ||
ArrayBase<<T as RawDataSubst<A>>::Output, D>: From<ArrayBase<S, D>>, // required by into() to convert from the DataMut array representation of S to the DataOwned array representation of T | ||
ArrayBase<T, D>: From<Array<B, D>>, // required by mapv() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice series of conversions on the whole.
The complexity of the trait requirements is outside my comfort zone. Frustrating and non-constructive, but: I don't think we found a good, clean, obvious solution for the problem here.
Other maintainers may of course differ in opinion, and they would then be welcome to merge it on their own.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate @bluss and @adamreichold's help in refining this pull request. @bluss, I understand where you are coming from in terms of not merging. If any other maintainers would like me to elaborate on the type conversions here (perhaps in the code comments) I would be happy to do so.
Note this is a breaking change that will require users of
mapv_into_any()
to tell the compiler, by annotating the return type, whether the returned array should be anArray
or anArcArray
.