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

possible bug in fenwick.jl #813

Open
StephenVavasis opened this issue May 21, 2022 · 2 comments
Open

possible bug in fenwick.jl #813

StephenVavasis opened this issue May 21, 2022 · 2 comments

Comments

@StephenVavasis
Copy link
Contributor

StephenVavasis commented May 21, 2022

There is a big discussion (https://discourse.julialang.org/t/offsetarrays-inbounds-and-confusion/81295) on discourse about why it is a bad idea to iterate over an AbstractVector, say a, using for i=1:length(a) (because this will fail if a is an OffsetArray). I checked the src file for DataStructures and found this style of code in fenwick.jl.

Update: this issue is also present in: append! for circular buffer and nextreme in heaps.jl,

@SyxP
Copy link

SyxP commented May 22, 2022

As much as I don't think that Fenwick is suitable in DataStructures.jl, part of the mechanism that makes Fenwick Trees fast is to exploit the contiguous small indexing range of 1...N or 0...N depending on the implementation. Hence, this is not really a suitable change for Fenwick Trees.

@StephenVavasis
Copy link
Contributor Author

OK, so then maybe the offending constructor in Fenwick should take a Vector instead of an AbstractVector?

Btw, should I split this issue into three separate issues, one for fenwick, one for circular buffer, and one for heaps? I'm not sure who is maintaining those codes.

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

2 participants