Cleanup roots of unity in KZGSettings #467
Merged
Conversation
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
b-wagn
reviewed
Aug 2, 2024
jtraglia
reviewed
Aug 2, 2024
jtraglia
reviewed
Aug 2, 2024
| * It is the reversed version of `expanded_roots_of_unity`. Essentially: | ||
| * `reverse_roots_of_unity = reverse(expanded_roots_of_unity)` | ||
| * | ||
| * This array is primarily used in FFTs. |
There was a problem hiding this comment.
I don't feel that this remark adds all that much.
There was a problem hiding this comment.
I added the python pseudocode in an attempt to highlight the difference between this and brp_roots_of_unity (both in terms of "reverse" vs "reverse-bit order" and in terms of excluding the last element).
With regards to "This array is primarily used in FFTs.", I was confused on why we need the reverse roots in the first place, so this would be useful to me, but I understand that it might be superfluous.
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
asn-d6
force-pushed
the
roots_of_unity_cleanup
branch
from
834bf8a
to
d065133
Compare
|
Thanks a lot for the help @jtraglia ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR makes some steps towards #439
Leftover things to do:
Completely removemax_width. This is not a variable field so it doesn't need to be inKZGSettings. It's a parameter of the system, and by having it as a variable it makes things more confusing. IMO we should just useFIELD_ELEMENTS_PER_EXT_BLOBdirectly where it's appropriate, andFIELD_ELEMENTS_PER_BLOBin FK20.domain_size + 1artifact stemming fromexpand_root_of_unity()since it's needless and confusing. I decided to not do it because it would also change the 4844 code and I would ideally like it to remain as it was.