Dev by Ko496-glitch · Pull Request #3 · Ko496-glitch/convolution

Ko496-glitch · 2026-02-19T15:46:42Z

Push into master

Copilot

Pull request overview

This pull request introduces a C++ implementation for benchmarking Toeplitz matrix-vector multiplication using the NTL library. The PR adds a basic benchmark framework comparing a naive O(n²) implementation against a planned O(n log n) Cooley method (currently incomplete). However, the PR contains several critical issues including committed binary files and code defects.

Changes:

Added phase1.cpp with Toeplitz matrix multiplication implementation and benchmarking framework
Added Makefile for building with NTL and GMP dependencies via Homebrew
Added .gitignore for NTL directory and .DS_Store files
Incorrectly committed two compiled binary files (phase1 and conv_impl.cpp)

Reviewed changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
phase1.cpp	Main source file implementing naive Toeplitz matrix multiplication, benchmarking utilities, and user input for performance testing
phase1	Compiled Mach-O binary that should not be in version control
conv_impl.cpp	Appears to be a misnamed compiled binary that should not be in version control
Makefile	Build configuration for compiling with clang++, NTL, and GMP libraries
.gitignore	Excludes NTL directory and .DS_Store, but missing binary exclusions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Ko496-glitch · 2026-02-19T18:23:40Z

@copilot open a new pull request to apply changes based on the comments in this thread

Copilot · 2026-02-19T18:23:49Z

@Ko496-glitch I've opened a new pull request, #4, to work on those changes. Once the pull request is ready, I'll request review from you.

[WIP] Add C++ benchmarking for Toeplitz matrix-vector multiplication

Updated project description and removed redundant lines.

Copilot

Pull request overview

Copilot reviewed 3 out of 6 changed files in this pull request and generated 8 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-08T23:16:49Z

+#include<iostream>
+#include<sstream>
+#include<iomanip>
+#include<string>


This file uses std::vector/std::pair and time(nullptr) but does not include the standard headers that define them. Add the appropriate includes (e.g., <vector>, <utility>, <ctime>) instead of relying on transitive includes.

Suggested change

#include<string>

#include<string>

#include<vector>

#include<utility>

#include<ctime>

Copilot · 2026-03-08T23:16:49Z

+  ZZ p;
+  GenPrime(p ,PRIME_BIT ,ERR_BOUND);
+  ZZ_p::init(p);
+  long t = 8;


long t = 8; is unused. Please remove it (or use it) to avoid warnings and keep main() focused.

Suggested change

long t = 8;

Copilot · 2026-03-08T23:16:50Z

+  while(start != end){
+
+       vec_ZZ_p input;
+       input.SetLength(start);
+
+       for(long i{};i< start; ++i){
+         random(input[i]);
+       }
+       vec_ZZ_p toep_mat;
+
+       toep_mat.SetLength(2*start-1);
+


User inputs are used directly as vector lengths (input.SetLength(start) and toep_mat.SetLength(2*start-1)) without validation. Negative/zero/very large values can trigger SetLength exceptions or huge allocations; validate start > 0, end bounds, and guard against overflow in 2*start-1 before allocating.

Copilot · 2026-03-08T23:16:50Z

+# Added Homebrew include path
+CXXFLAGS = -O3 -std=c++17 -I./NTL/include -I/opt/homebrew/include
+# Added Homebrew library path
+LDFLAGS = -L./NTL/src -L/opt/homebrew/lib -lntl -lgmp
+


The build hardcodes Homebrew-specific include/library paths (/opt/homebrew/...) and references ./NTL/... even though NTL/ is ignored in .gitignore and not present in the repo. Make the dependency discovery configurable (e.g., via NTL_PREFIX/GMP_PREFIX variables or pkg-config) so the project builds on non-macOS setups and in CI.

Suggested change

# Added Homebrew include path

CXXFLAGS = -O3 -std=c++17 -I./NTL/include -I/opt/homebrew/include

# Added Homebrew library path

LDFLAGS = -L./NTL/src -L/opt/homebrew/lib -lntl -lgmp

# Optional prefixes for NTL and GMP installations.

# Users/CI can set these when invoking make, e.g.:

# make NTL_PREFIX=/usr/local GMP_PREFIX=/usr

NTL_PREFIX ?=

GMP_PREFIX ?=

CXXFLAGS = -O3 -std=c++17

LDFLAGS = -lntl -lgmp

# If NTL_PREFIX is provided, add its include/lib directories

ifneq ($(NTL_PREFIX),)

CXXFLAGS += -I$(NTL_PREFIX)/include

LDFLAGS += -L$(NTL_PREFIX)/lib

endif

# If GMP_PREFIX is provided, add its include/lib directories

ifneq ($(GMP_PREFIX),)

CXXFLAGS += -I$(GMP_PREFIX)/include

LDFLAGS += -L$(GMP_PREFIX)/lib

endif

Copilot · 2026-03-08T23:16:50Z

+/* O(n^2) compute for toeplitz matrix with input vector Benchmarking against Cooley method */
+/* from 1d -> 2d we use V[(n-1) + ( j- i) * xj)] j: 0 -> n */


This comment appears to have incorrect/mismatched notation (e.g., * xj)]) and an unclear range (j: 0 -> n usually means inclusive). Please rewrite it to accurately describe the Toeplitz indexing being used so future readers can verify correctness.

Suggested change

/* O(n^2) compute for toeplitz matrix with input vector Benchmarking against Cooley method */

/* from 1d -> 2d we use V[(n-1) + ( j- i) * xj)] j: 0 -> n */

/* O(n^2) compute for Toeplitz matrix–vector multiplication (benchmark against Cooley method) */

/* Toeplitz indexing: tope_mat[(n - 1) + (j - i)] stores T[i,j] for 0 <= i,j < n */

Copilot · 2026-03-08T23:16:50Z

+#if 0
+/* optimized mutliplation of tope_mat * input vector. O(n log n). Would be benchmark against mul_norm */
+vec_ZZ_p cooley_mul(const vec_ZZ_p &input, const vec_ZZ_p &tope_mat, long n ){
+    vec_zz_p res;
+
+    for(long i{};i<n;++i){
+
+    }
+    return res;
+}
+
+#endif
+


There’s an unfinished #if 0 block for cooley_mul (including a likely typo vec_zz_p vs vec_ZZ_p). If this is not intended to be implemented in this PR, consider removing it or converting it to a tracked TODO/issue to avoid accumulating dead, misleading code.

Suggested change

#if 0

/* optimized mutliplation of tope_mat * input vector. O(n log n). Would be benchmark against mul_norm */

vec_ZZ_p cooley_mul(const vec_ZZ_p &input, const vec_ZZ_p &tope_mat, long n ){

vec_zz_p res;

for(long i{};i<n;++i){

}

return res;

}

#endif

// TODO: Add an optimized Toeplitz * vector multiplication (e.g., Cooley-based) and

// benchmark it against mul_norm if/when such an implementation is required.

Copilot · 2026-03-08T23:16:50Z

+void routine_check(const vec_ZZ_p &toep_mat, const vec_ZZ_p &input, long n, Func mul_function){
+
+    long double curr_time = GetTime();
+    vec_ZZ_p mat  = mul_function(input,toep_mat, n);


routine_check computes mat but never uses it, which will generate an unused-variable warning and obscures intent. Either use mat (e.g., correctness check / prevent optimization) or omit it and only time the call.

Suggested change

vec_ZZ_p mat = mul_function(input,toep_mat, n);

mul_function(input,toep_mat, n);

Copilot · 2026-03-08T23:16:51Z

+
+constexpr const long PRIME_BIT  = 60;
+constexpr const long ERR_BOUND =  80;
+double total_time{};


total_time is a double while individual timings are handled as long double. This mixes precisions and will truncate when accumulating; consider making the types consistent (either long double everywhere or double everywhere).

Suggested change

double total_time{};

long double total_time{};

Ko496-glitch added 7 commits February 16, 2026 23:48

seed random and toeplitz

dbdaf5c

change constants for security

c660ba3

wrote mul_norm for brute force

0946b76

Makefile for the project/ clang

ce72a43

added the full functionality for benchmarking naive-O(n^2)

3f727d0

Added the print syntax and update for loop

a987b93

Added user input and cleaned up phase1

39fb49a

Copilot AI review requested due to automatic review settings February 19, 2026 15:46

Copilot started reviewing on behalf of Ko496-glitch February 19, 2026 15:47 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Comment thread phase1.cpp

Initial plan

fb04a6a

Copilot AI mentioned this pull request Feb 19, 2026

[WIP] Add C++ benchmarking for Toeplitz matrix-vector multiplication #4

Merged

7 tasks

Ko496-glitch added 2 commits February 19, 2026 13:24

Merge pull request #4 from Ko496-glitch/copilot/sub-pr-3

2cf0f1d

[WIP] Add C++ benchmarking for Toeplitz matrix-vector multiplication

Revise README with project details

f220595

Updated project description and removed redundant lines.

Copilot AI review requested due to automatic review settings March 8, 2026 23:13

Copilot started reviewing on behalf of Ko496-glitch March 8, 2026 23:13 View session

Copilot AI reviewed Mar 8, 2026

View reviewed changes

Ko496-glitch added 3 commits March 8, 2026 19:40

readme update

91815e3

Adding rust version for performance

6ef6a47

Merge branch 'master' into dev

3f15866

Ko496-glitch merged commit 591daf2 into master Mar 14, 2026

-#include<string>
+#include<string>
+#include<vector>
+#include<utility>
+#include<ctime>

-# Added Homebrew include path
-CXXFLAGS = -O3 -std=c++17 -I./NTL/include -I/opt/homebrew/include
-# Added Homebrew library path
-LDFLAGS = -L./NTL/src -L/opt/homebrew/lib -lntl -lgmp
+# Optional prefixes for NTL and GMP installations.
+# Users/CI can set these when invoking make, e.g.:
+#   make NTL_PREFIX=/usr/local GMP_PREFIX=/usr
+NTL_PREFIX ?=
+GMP_PREFIX ?=
+CXXFLAGS = -O3 -std=c++17
+LDFLAGS = -lntl -lgmp
+# If NTL_PREFIX is provided, add its include/lib directories
+ifneq ($(NTL_PREFIX),)
+CXXFLAGS += -I$(NTL_PREFIX)/include
+LDFLAGS  += -L$(NTL_PREFIX)/lib
+endif
+# If GMP_PREFIX is provided, add its include/lib directories
+ifneq ($(GMP_PREFIX),)
+CXXFLAGS += -I$(GMP_PREFIX)/include
+LDFLAGS  += -L$(GMP_PREFIX)/lib
+endif

		/* O(n^2) compute for toeplitz matrix with input vector Benchmarking against Cooley method */
		/* from 1d -> 2d we use V[(n-1) + ( j- i) * xj)] j: 0 -> n */

	vec_ZZ_p mat = mul_function(input,toep_mat, n);
	mul_function(input,toep_mat, n);

Conversation

Ko496-glitch commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ko496-glitch commented Feb 19, 2026

Uh oh!

Copilot AI commented Feb 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants