-
Notifications
You must be signed in to change notification settings - Fork 45
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 LIFO/HIFO accounting methods O(n*log(m)) #116
Conversation
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 looks really interesting. I have only skimmed it for now, but I'm providing some early feedback.
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 get the gist of what you're trying to do (use priority queues for hifo and lifo, instead of lists) and I think it holds a lot of promise. I went through the code and I'm providing some initial feedback: feel free to push back if any of my comments don't make sense to you (the code is still in progress and I may have misunderstood a few things).
Unit tests are now passing (other than a new false alarm on an external link) |
Oh, fantastic! Let me take another look. |
@eprbell Refactor is complete and all tests are passing, but I'm guessing we'll need a round of cleanup or two. I didn't see your suggestion on |
Great! I'll review. |
6db4677
to
14db355
Compare
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 have a few comments and questions here and there, but this looks really good: it'll be very beneficial to people with lots of transactions. We will also have to update the [plugin developer documentation] to mention the two plugin flavors (list vs heap).
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.
LGTM! There are a couple of minor things still TBD (like updating the dev docs), but we shouldn't get blocked on that (I can work on that separately). Thanks again for this awesome set of optimizations!
The
LIFO
andHIFO
methods are unfortunately quadratic time (first test is trivial forLIFO
):The cases are the same as in #115 and assume the
FIFO
improvements there are merged.The overall approach is to filter out exhausted lots early, so they do not need to be considered in each pass. In
FIFO
, this can be done by keeping track of another index. InLIFO
andHIFO
we introduce a heap where "active" lots are kept. The generalfor
loop structure is kept to remove exhausted lots (as may occur when switching accounting methods across years), with the first non-exhausted lot guaranteed to be the one we are searching for. The use of heaps improves each loop toO(log m)
.We translate the search criteria into a
heap_key
as Python only natively supports min heaps. It is likely that this key will need to be refined to guarantee deterministic behavior.Due to test failures, this is not ready to merge but posting to start getting feedback.
And the results with this PR: