Remove testing methods from Honk + clean up #177
Conversation
| // get the transposition. | ||
| // Ex: polynomial_spans[3][i] returns the i-th coefficient of the third polynomial | ||
| // in the list below | ||
| std::array<std::span<const fr>, num_polynomials> univariate_array; |
There was a problem hiding this comment.
Any particular reason why we're calling these univariates? I suppose the relations are a bit representation agnostic but for sumcheck at large we think of them as multivariates
There was a problem hiding this comment.
Accidentally copied the previous name without thinking. Will rename it to something better!
| @@ -19,11 +18,12 @@ template <class FF> class SumcheckRelation : public testing::Test { | |||
| public: | |||
| template <size_t t> using Univariate = Univariate<FF, t>; | |||
| template <size_t t> using UnivariateView = UnivariateView<FF, t>; | |||
| static constexpr size_t FULL_RELATION_LENGH = 5; | |||
There was a problem hiding this comment.
Definitely understand if you don't want to do this in this PR but just some thoughts: This FULL_RELATION_LENGH is necessarily tied to the actual full relation length so the hard coding is kind of brittle and has tripped me up on a few occasions. It would be nice to compute this programmatically instead then updatecompute_mock_extended_edges to create appropriately sized mock univariates. (We could do this by either directly generating the full sized univariate or just defining length 2 univariates then extending them appropriately).
There was a problem hiding this comment.
Agreed that this would be nice. I think that we can get this done as part of Arijit's PR since he introduced a lot more functionality to the way the test is performed. Left a more detailed TODO for now
| @@ -309,4 +295,67 @@ template <class Fr, size_t view_length> class UnivariateView { | |||
| return os; | |||
| } | |||
| }; | |||
|
|
|||
There was a problem hiding this comment.
These methods don't seem to be used anywhere. You see some use for them in the future?
There was a problem hiding this comment.
The main use case is to convert an array of Univariate into UnivariateView. The main use case would be to let Sumcheck decide the required degree of the relation evaluation, rather than hardcoding it inside the relation. The _aux version could also be used to create an array of only the polynomials required by the relation, and it could help us implement the optimization where we extend each edge only up to the maximum degree that is required over all relations (for example, L_LAST only needs degree 3).
I'll save this comment as the context might help.
| EXPECT_EQ(std::get<1>(results), GrandProductComputationUnivariate(0)); | ||
| EXPECT_EQ(std::get<2>(results), GrandProductInitializationUnivariate(0)); | ||
| // // Compute an array containing all the evaluations at a given row i | ||
| std::array<fr, num_polynomials> transposed; |
There was a problem hiding this comment.
Isn't this operation that you created transposed_univariate_array_at for?
There was a problem hiding this comment.
The problem was that that function only worked over Univariate since it doesn't have the operator[] defined. I moved it to the test it was being used it. In retrospect it may have been a bit too generic and the following loop is simple enough.
|
Fixes #182 btw |
…rg#177) * Initial commit * Luke's comments
…rg#177) * Initial commit * Luke's comments
Description
_testingmethods anymore.#pragma GCCand fixed errors.Univariateoperator interfaces to beconstcorrect, and return references when required ref.Checklist:
/markdown/specshave been updated.@briefdescribing the intended functionality.