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

Wrap AbstractArray rather than Array in SampleBuf? #60

Open
mchitre opened this issue Mar 20, 2020 · 13 comments
Open

Wrap AbstractArray rather than Array in SampleBuf? #60

mchitre opened this issue Mar 20, 2020 · 13 comments
Milestone

Comments

@mchitre
Copy link

mchitre commented Mar 20, 2020

Currently SampleBuf wraps an Array. This disallows wrapping of views or other array-like data types, for example. Is there any strong reason why not wrap an AbstractArray instead?

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

Fixing this might allow nice adding constructs like @view buf[pos1:pos2] for buf::SampleBuf to also be a SampleBuf, and therefore retain sampling rate metadata.

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

I've implemented this for testing, and seems to work well:
mchitre@2b72081

If you don't see any problems with this change, and are open to it, I can raise a PR.

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

Yeah, I don't have any problem with that at all, seems like a good improvement.

For your view example you'd have a SampleBuf that wrapped a view into the original data, right?

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

Correct

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

Great, that's quite nice. Thanks!

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 20, 2020

Also note that changing this view behavior would be a breaking change, so I'll add it to the list for the next breaking release.

@ssfrr ssfrr added this to the v3.0 milestone Mar 20, 2020
@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

I've included the AbstractArray change in the same PR, as it is quite minor. I'll do a separate PR later for the @view change, once I've implemented, tested and am happy with it being robust.

@haberdashPI
Copy link
Contributor

I think I did something similar with the large PR I wrote a while back, might be worth checking in there to see if there's anything else you want to use.

@mchitre
Copy link
Author

mchitre commented Mar 20, 2020

Actually turned out to be simple enough. I've added it to the same PR but in a different commit (in case you want to release them in separate version). I've added a test case to cover it too.

@mchitre
Copy link
Author

mchitre commented Mar 22, 2020

I realize that wrapping an AbstractArray may not be such a good idea for type inference [1]. I haven't thought through this fully, and figured it was better to drop this from the PR until the best design approach discussed/explored.

@ssfrr
Copy link
Collaborator

ssfrr commented Mar 22, 2020

Yeah, rather than actually wrapping AbstractArray the wrapped type should be given as a type parameter. Otherwise indexing etc. will be very slow.

@mchitre
Copy link
Author

mchitre commented Mar 22, 2020

Yup. But since this is encoded in AbstractSampleBuf{T,N}, the implications of re-parametrizing for the remaining code base may be substantial.

@sadish-d
Copy link

sadish-d commented Aug 1, 2024

Could we consider wrapping Union{Array, SubArray} for now? I think this would be useful and potentially not be much worse in performance than the current implementation.

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

No branches or pull requests

4 participants