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

Allow converting encoded samples if the element type matches #171

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

ararslan
Copy link
Member

At present, we error on convert(::Type{Samples{A}}, ::Samples{B}) for all A and B if the samples are encoded. This is reasonable since we don't want to affect how the samples get decoded. However, decoding should be based on the type of the encoded samples themselves rather than of their container; a decoder shouldn't care whether the incoming data is e.g. a full matrix or a view of a matrix. Thus we can allow conversion for encoded samples provided that only the container type is affected, not the element type.

cc @rebareh, who ran into this in some private Beacon code.

At present, we error on `convert(::Type{Samples{A}}, ::Samples{B})` for
all `A` and `B` if the samples are encoded. This is reasonable since we
don't want to affect how the samples get decoded. However, decoding
should be based on the type of the encoded samples themselves rather
than of their container; a decoder shouldn't care whether the incoming
data is e.g. a full matrix or a view of a matrix. Thus we can allow
conversion for encoded samples provided that only the container type is
affected, not the element type.
Copy link
Member

@ericphanson ericphanson left a comment

Choose a reason for hiding this comment

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

I think it should be totally fine as long as it validates (which is checked on construction by default anyway), which means the type matches the samples info (when encoded) which I think ends up the same as this explicit check.

@ararslan ararslan merged commit 4153b50 into main Sep 13, 2024
6 checks passed
@ararslan ararslan deleted the aa/convert-encoded branch September 13, 2024 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants