-
Notifications
You must be signed in to change notification settings - Fork 92
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
Implemented updateMany
method as required in issue #117
#314
Merged
vplasencia
merged 13 commits into
privacy-scaling-explorations:main
from
ChinoCribioli:main
Sep 9, 2024
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
130b977
feat(lean-imt): added `updateMany` method to package
ChinoCribioli 6049ea4
feat(lean-imt): implemented some tests on lean-imt
ChinoCribioli e00a891
feat(lean-imt): added more precondition checks
ChinoCribioli 3e9e54b
feat(lean-imt): finished testing on `updateMany` method
ChinoCribioli cd3156e
feat(lean-imt): added test to the case when passing repeated indices
ChinoCribioli 06e941c
feat(lean-imt): added complexity documentation for `updateMany` method
ChinoCribioli 4b51e44
feat(lean-imt): added test of several updates
ChinoCribioli 29638d5
feat(lean-imt): added repeated indices check
ChinoCribioli 880812c
feat(lean-imt): changed error message to be more accurate
ChinoCribioli 681239e
feat(lean-imt): added complexity in terms only of n
ChinoCribioli 1bfffd4
feat(lean-imt): changed documentation to add discussion in another issue
ChinoCribioli bd67b35
feat(lean-imt): fixed typo on documentation
ChinoCribioli c5e836d
Update packages/lean-imt/src/lean-imt.ts
ChinoCribioli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Re the time complexity of the algorithm.
n: Number of leaves of the tree.
I think that the worst case for updating multiple elements at once is to update all the leaves. Then the number of possible leaves to update is bounded by n.
Then the time complexity of the
update
function in a loop for the worst case would be O(n log n). Because you will need to go from a leaf to the root (log n
operations) n times.I think that the worst case for this
updateMany
function is O(n) because since you would be updating all the leaves, it's like rebuilding the tree. Rebuilding the tree is O(n) because the number of operations when building a tree from the leaves isn + ceiling(n/2) + ceiling(n/4) + ... + ceiling(n/(2^k))
that's<=
2*n + O(log n)
and that's<=
O(n) + O(log n)
which isO(n)
.Number of operations when building the tree (or updating all the elements in the tree at once):
That is the same as$$\sum_{k=0}^{d} \lceil \frac{n}{2^k} \rceil$$
For more reference on this, you can take a look at the
InsertMany
time complexity in the LeanIMT paper: https://github.com/vplasencia/leanimt-paper/blob/main/paper/leanimt-paper.pdfDoes it make sense to explain it like this?
If you use
n
instead ofm
, you will also get O(n). Does it make sense to modify your proof assuming that the worst case form
isn
since we are calculating big O notation?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.
If you set
m = n
, the complexity as I stated it becomesO(n)
sincelog(n)-log(m) = 0
. I think it makes sense to add this case separately to the complexity statement, but maintaining the whole explanation with a smallerm
.That is, my proof is a generalization of the case
m=n
, so I don't see why we should take that out. Completely agree with mentioning that the algorithm isO(n)
, though.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.
Isn't the worst case the one when the nodes to update have as few ancestors as possible on all levels? In that case, time complexity would be quite similar to the one of the
update
function in a loop, i.e. O(n * log n).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 also meant
n
as the subset of nodes to be updated (and not all leaves). We are on the same page 👍🏽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.
Yes, but as the number of leaves to update increases, the condition of "having few common ancestors" starts being stronger and stronger, to the point where common ancestors start appearing inevitably. This makes that we never reach
n*log(n)
operations.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.
Hey @ChinoCribioli could you update the
updateMany
function documentation with something similar to what you have at the beginning and move the time complexity analysis to a new issue called something likeUpdateMany time complexity
and add a comment with your proof so that we can discuss it there. Then, if you want, you could add it to the LeanIMT paper. What do you think?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.
@vplasencia Done. I changed the documentation to a more minimalist description and created this issue to discuss the deeper complexity analysis.
After the discussion is closed, you may reach out to me to add the whole analysis to the LeanIMT paper.
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.
Great! Thank you very much! 🙏